954972 - Handle 432: Erroneous nickname

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[aleth [:aleth]]

*** Original post on bio 1540 at 2012-06-23 10:26:00 UTC ***

*** Due to BzAPI limitations, the initial description is in comment 1 ***

Comment 1

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1664 at 2012-06-23 10:26:00 UTC ***

Handles illegal nicknames. If in response to a /nick command, revert to the account nickname. If it happens while connecting, throw the appropriate error.

Comment 2

[Patrick Cloke [:clokep]]

*** Original post on bio 1540 at 2012-06-23 11:44:00 UTC ***

Can you explain your changes to ircMessage please? We shouldn't take changes to that lightly. Please give examples of where it didn't work and now does work.

Comment 3

[aleth [:aleth]]

*** Original post on bio 1540 at 2012-06-23 13:21:17 UTC ***

A 432 message usually looks like this:
:gravel.mozilla.org 432 aleth #momo :Erroneous Nickname: Illegal characters

But when we are connecting to the server and send an incorrect nick, the server has no current nick to send, so inserts an empty string:
:gravel.mozilla.org 432  #momo :Erroneous Nickname: Illegal characters

This causes the ircMessage parser to fail as the regexp doesn't match this message.

As per the comment in ircMessage, it was the most minimal change I could think of to handle this edge case.

Comment 4

[aleth [:aleth]]

Comment on attachment 8353421 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1664 at 2012-06-23 13:25:11 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1667 at 2012-06-23 13:50:00 UTC ***

There are actually two server behaviours I have observed:
The first (e.g. mozilla.org) is as described above and requires special-casing.
The second (e.g. freenode) inserts a "*" for the blank parameter, which does not break the usual message format.

Comment 6

[aleth [:aleth]]

Comment on attachment 8353421 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1664 at 2012-06-23 13:50:41 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 7

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1668 at 2012-06-23 14:04:00 UTC ***

I believe this is even cleaner.

Comment 8

[aleth [:aleth]]

Comment on attachment 8353424 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1667 at 2012-06-23 14:04:19 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1669 at 2012-06-23 14:12:00 UTC ***

This was supposed to be the previous patch ;)

Comment 10

[aleth [:aleth]]

Comment on attachment 8353425 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1668 at 2012-06-23 14:12:35 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1670 at 2012-06-23 14:14:00 UTC ***

Nit fix.

Comment 12

[aleth [:aleth]]

Comment on attachment 8353426 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1669 at 2012-06-23 14:14:52 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1675 at 2012-06-23 18:40:00 UTC ***

Save 2 lines.

Comment 14

[aleth [:aleth]]

Comment on attachment 8353427 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1670 at 2012-06-23 18:40:56 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1689 at 2012-06-25 12:16:00 UTC ***

Remove duplication with the latest patch in bug 954970 (bio 1538).

Comment 16

[aleth [:aleth]]

Comment on attachment 8353432 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1675 at 2012-06-25 12:16:46 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 17

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1691 at 2012-06-25 13:12:00 UTC ***

_originalNickname -> _requestedNickname

Comment 18

[aleth [:aleth]]

Comment on attachment 8353446 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1689 at 2012-06-25 13:12:28 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 19

[Patrick Cloke [:clokep]]

Attachment: Alterate ircMessage change

*** Original post on bio 1540 as attmnt 1697 at 2012-06-26 00:33:00 UTC ***

This is an alternative change to ircMessage which makes the initial regular expression slightly more aggressive instead of having a secondary check for the message.

Also, we should add a test for this particular message.

Comment 20

[Patrick Cloke [:clokep]]

*** Original post on bio 1540 at 2012-06-26 00:34:22 UTC ***

(In reply to comment #10)
> Created attachment 8353454 [details] [diff] [review] (bio-attmnt 1697) [details]
> Alterate ircMessage change
> 
> This is an alternative change to ircMessage which makes the initial regular
> expression slightly more aggressive instead of having a secondary check for the
> message.

One other nice thing about this change is that it returns an /empty/ parameter as the nickname return, which seems more "correct" to me (instead of arbitrarily putting a value in there).

Comment 21

[aleth [:aleth]]

*** Original post on bio 1540 at 2012-06-26 09:51:47 UTC ***

(In reply to comment #10)
> Created attachment 8353454 [details] [diff] [review] (bio-attmnt 1697) [details]
> Alterate ircMessage change
> 
> This is an alternative change to ircMessage which makes the initial regular
> expression slightly more aggressive instead of having a secondary check for the
> message.

That patch looks more elegant, but also less safe.

Currently the regex fails for example on
":gravel.mozilla.org MODE #tckk +n "
(as sent by Unreal).

This is fixable by using 
/^(?::([^ ]+) )?([^ ]+)((?: +[^: ][^ ]*)*)? ?(?: ?:(.*))?$/
but can we be reasonably sure there aren't other such cases?

>One other nice thing about this change is that it returns an /empty/ parameter
>as the nickname return, which seems more "correct" to me (instead of
>arbitrarily putting a value in there).

I'm not sure this is a win actually, as it's not correct according to the RFC. Inserting a dummy parameter "*" seems more correct to me in terms of what the subsequent IRC code will expect.

Comment 22

[aleth [:aleth]]

*** Original post on bio 1540 at 2012-06-26 11:37:39 UTC ***

(In reply to comment #12)
> This is fixable by using 
> /^(?::([^ ]+) )?([^ ]+)((?: +[^: ][^ ]*)*)? ?(?: ?:(.*))?$/
> but can we be reasonably sure there aren't other such cases?

So I was initially worried by the mere existence of things like trailing spaces, but I haven't been able to find any further problems in testing, which makes me feel a bit more optimistic about this :)

> >One other nice thing about this change is that it returns an /empty/ parameter
> >as the nickname return, which seems more "correct" to me (instead of
> >arbitrarily putting a value in there).
> 
> I'm not sure this is a win actually, as it's not correct according to the RFC.
> Inserting a dummy parameter "*" seems more correct to me in terms of what the
> subsequent IRC code will expect.

To be clear: I don't really care what we pass for parameters that won't be used ;) I was thinking of the off-chance that an |if (msg.params[1])| type check fails when it shouldn't.

Comment 23

[Patrick Cloke [:clokep]]

*** Original post on bio 1540 at 2012-06-26 12:01:59 UTC ***

(In reply to comment #13)
> (In reply to comment #12)
> > This is fixable by using 
> > /^(?::([^ ]+) )?([^ ]+)((?: +[^: ][^ ]*)*)? ?(?: ?:(.*))?$/
> > but can we be reasonably sure there aren't other such cases?
> 
> So I was initially worried by the mere existence of things like trailing
> spaces, but I haven't been able to find any further problems in testing, which
> makes me feel a bit more optimistic about this :)
OK, please integrate our patches together then (please keep my changes to ircMessage, add your fix for the regular expression (the extra " ?"), and expand the comment saying that we also allow an extra trailing space before the last parameter. Btw, we could potentially get rid of the " ?" inside the last parameter check then.

Unfortunately you can't run tests, so I'll need to write a few tests for this and then I'll be OK checking this in, assuming that all the tests pass. :)

Comment 24

[Patrick Cloke [:clokep]]

Comment on attachment 8353448 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1691 at 2012-06-26 12:02:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 25

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1699 at 2012-06-26 12:18:00 UTC ***

I'm glad you found a way to get away with a single regex :)

Also added an ERROR when a message can't be parsed, just in case some weirdness like the 432 Unreal issue shows up again in the future.

Comment 26

[aleth [:aleth]]

Comment on attachment 8353448 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1691 at 2012-06-26 12:18:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 27

[aleth [:aleth]]

Comment on attachment 8353454 [details] [diff] [review]
Alterate ircMessage change

*** Original change on bio 1540 attmnt 1697 at 2012-06-26 12:18:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 28

[aleth [:aleth]]

Comment on attachment 8353456 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1699 at 2012-06-26 12:22:21 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 29

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1700 at 2012-06-26 12:24:00 UTC ***

Unfortunate missing space.

Comment 30

[aleth [:aleth]]

Comment on attachment 8353456 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1699 at 2012-06-26 12:24:38 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 31

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1701 at 2012-06-26 12:27:00 UTC ***

And ?. Sorry about that.

Comment 32

[aleth [:aleth]]

Comment on attachment 8353457 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1700 at 2012-06-26 12:27:19 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 33

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1705 at 2012-06-26 19:58:00 UTC ***

Style nit: return early in case of error.

Comment 34

[aleth [:aleth]]

Comment on attachment 8353458 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1701 at 2012-06-26 19:58:37 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 35

[Patrick Cloke [:clokep]]

Comment on attachment 8353462 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1705 at 2012-06-26 20:20:17 UTC ***

>diff --git a/modules/ircBase.jsm b/modules/ircBase.jsm
>@@ -1126,18 +1126,27 @@ var ircBase = {
>     "432": function(aMessage) { // ERR_ERRONEUSNICKNAME
>       // <nick> :Erroneous nickname
>-      // TODO Prompt user for new nick? Autoclean characters?
>-      return false;
>+      let msg = _("error.erroneousNickname", aMessage.params[1]);
>+      if (this._requestedNickname == this._accountNickname) {
>+        // The account has been set up with an illegal nickname.
>+        ERROR("Erroneous nickname");
Please include the nickname and the message from the server (params[2]) here. I think the server usually gives more information (have you seen this? If not then forget that part). So something like:
ERROR("Erroneous nickname: " + aMessage.params[1] + "\nReason: " + aMessage.params[2]);

>+        this.gotDisconnected(Ci.prplIAccount.ERROR_INVALID_USERNAME, msg);
>+        return true;
>+      }
>+      // Reset original nickname to the account nickname in case of
>+      // later reconnections.
>+      this._requestedNickname = this._accountNickname;
>+      return serverErrorMessage(this, aMessage, msg);
>     },

Shouldn't this error be shown ALWAYS, not just in the case that the account was set up w/ a bad nick? We're resetting the requested nickname here so that the one on the /nick command isn't tried to be used when we reconnect?

I'd rather better user feedback, but I'm not sure how to do that. :(

Comment 36

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1706 at 2012-06-26 20:40:00 UTC ***

(In reply to comment #19)
> I think the server usually gives more information (have you seen this? If not
> then forget that part).

Yes, I saw it. I've now included it in the ERROR message, but had not put it in the directly user-visible error message as its form and usefulness varies too much across servers. Usually it just reads "Erroneous nickname" anyway.

> Shouldn't this error be shown ALWAYS, not just in the case that the account 
> was set up w/ a bad nick? 

I don't think we usually throw ERRORS when a command fails because the user adds some silly parameters? We write to the server tab.

> We're resetting the requested nickname here so that the
> one on the /nick command isn't tried to be used when we reconnect?

Yes, undoing the change of nick in response to /nick.

> I'd rather better user feedback, but I'm not sure how to do that. :(

It's OK for incorrect accounts - you get it nicely in the account manager. For /nick, it would be nicer to have it in the conversation than the server tab, but that's the usual IRC problem of knowing from where a command was entered. Maybe that should be fixed somehow at some point for all such error responses if possible...

Comment 37

[aleth [:aleth]]

Comment on attachment 8353462 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1705 at 2012-06-26 20:40:25 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 38

[Patrick Cloke [:clokep]]

Comment on attachment 8353463 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1706 at 2012-06-26 23:01:04 UTC ***

This looks good and we can take this change.

This passes all current tests, but we should write specific tests for the given situations here.

Comment 39

[aleth [:aleth]]

Attachment: Patch

*** Original post on bio 1540 as attmnt 1707 at 2012-06-26 23:25:00 UTC ***

Move serverErrorMessage call above a potential gotDisconnected call.

Comment 40

[aleth [:aleth]]

Comment on attachment 8353463 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1706 at 2012-06-26 23:25:50 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 41

[Patrick Cloke [:clokep]]

Comment on attachment 8353464 [details] [diff] [review]
Patch

*** Original change on bio 1540 attmnt 1707 at 2012-06-26 23:30:32 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 42

[Patrick Cloke [:clokep]]

*** Original post on bio 1540 at 2012-06-26 23:50:26 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/66d00848de1f

Comment 43

[Patrick Cloke [:clokep]]

Attachment: Tests

*** Original post on bio 1540 as attmnt 1722 at 2012-06-29 20:45:00 UTC ***

What do you think of these tests aleth?

Comment 44

[aleth [:aleth]]

Comment on attachment 8353480 [details] [diff] [review]
Tests

*** Original change on bio 1540 attmnt 1722 at 2012-06-29 20:52:18 UTC ***

This looks good to me, but I can't test the test, so bouncing across to flo for a final check.

Comment 45

[Florian Quèze [:florian]]

Comment on attachment 8353480 [details] [diff] [review]
Tests

*** Original change on bio 1540 attmnt 1722 at 2012-07-11 11:34:18 UTC ***

Looks OK, minus the trailing whitespace that I removed before commiting the patch ;).

Comment 46

[Patrick Cloke [:clokep]]

*** Original post on bio 1540 at 2012-07-11 11:49:34 UTC ***

(In reply to comment #26)
> Comment on attachment 8353480 [details] [diff] [review] (bio-attmnt 1722) [details]
> Tests
> 
> Looks OK, minus the trailing whitespace that I removed before commiting the
> patch ;).

Sorry about that. :( Thanks for removing it!