Closed
Bug 954972
Opened 10 years ago
Closed 10 years ago
Handle 432: Erroneous nickname
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.2
People
(Reporter: aleth, Assigned: aleth)
References
Details
Attachments
(2 files, 14 obsolete files)
6.20 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
2.12 KB,
patch
|
aleth
:
review+
florian
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 1540 at 2012-06-23 10:26:00 UTC *** *** Due to BzAPI limitations, the initial description is in comment 1 ***
Assignee | ||
Comment 1•10 years ago
|
||
*** 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.
Attachment #8353421 -
Flags: review?(clokep)
Comment 2•10 years ago
|
||
*** 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.
Assignee | ||
Comment 3•10 years ago
|
||
*** 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.
Assignee | ||
Comment 4•10 years ago
|
||
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 ***
Attachment #8353421 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 5•10 years ago
|
||
*** 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.
Attachment #8353424 -
Flags: review?(clokep)
Assignee | ||
Comment 6•10 years ago
|
||
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 ***
Attachment #8353421 -
Attachment is obsolete: true
Assignee | ||
Comment 7•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1668 at 2012-06-23 14:04:00 UTC *** I believe this is even cleaner.
Attachment #8353425 -
Flags: review?(clokep)
Assignee | ||
Comment 8•10 years ago
|
||
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 ***
Attachment #8353424 -
Attachment is obsolete: true
Attachment #8353424 -
Flags: review?(clokep)
Assignee | ||
Comment 9•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1669 at 2012-06-23 14:12:00 UTC *** This was supposed to be the previous patch ;)
Attachment #8353426 -
Flags: review?(clokep)
Assignee | ||
Comment 10•10 years ago
|
||
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 ***
Attachment #8353425 -
Attachment is obsolete: true
Attachment #8353425 -
Flags: review?(clokep)
Assignee | ||
Comment 11•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1670 at 2012-06-23 14:14:00 UTC *** Nit fix.
Attachment #8353427 -
Flags: review?(clokep)
Assignee | ||
Comment 12•10 years ago
|
||
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 ***
Attachment #8353426 -
Attachment is obsolete: true
Attachment #8353426 -
Flags: review?(clokep)
Assignee | ||
Comment 13•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1675 at 2012-06-23 18:40:00 UTC *** Save 2 lines.
Attachment #8353432 -
Flags: review?(clokep)
Assignee | ||
Comment 14•10 years ago
|
||
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 ***
Attachment #8353427 -
Attachment is obsolete: true
Attachment #8353427 -
Flags: review?(clokep)
Assignee | ||
Comment 15•10 years ago
|
||
*** 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).
Attachment #8353446 -
Flags: review?(clokep)
Assignee | ||
Comment 16•10 years ago
|
||
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 ***
Attachment #8353432 -
Attachment is obsolete: true
Attachment #8353432 -
Flags: review?(clokep)
Assignee | ||
Comment 17•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1691 at 2012-06-25 13:12:00 UTC *** _originalNickname -> _requestedNickname
Attachment #8353448 -
Flags: review?(clokep)
Assignee | ||
Comment 18•10 years ago
|
||
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 ***
Attachment #8353446 -
Attachment is obsolete: true
Attachment #8353446 -
Flags: review?(clokep)
Comment 19•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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).
Assignee | ||
Comment 21•10 years ago
|
||
*** 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.
Assignee | ||
Comment 22•10 years ago
|
||
*** 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•10 years ago
|
||
*** 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•10 years ago
|
||
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 ***
Attachment #8353448 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 25•10 years ago
|
||
*** 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.
Attachment #8353456 -
Flags: review?(clokep)
Assignee | ||
Comment 26•10 years ago
|
||
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 ***
Attachment #8353448 -
Attachment is obsolete: true
Assignee | ||
Comment 27•10 years ago
|
||
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 ***
Attachment #8353454 -
Attachment is obsolete: true
Assignee | ||
Comment 28•10 years ago
|
||
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 ***
Attachment #8353456 -
Flags: review?(clokep)
Assignee | ||
Comment 29•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1700 at 2012-06-26 12:24:00 UTC *** Unfortunate missing space.
Attachment #8353457 -
Flags: review?(clokep)
Assignee | ||
Comment 30•10 years ago
|
||
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 ***
Attachment #8353456 -
Attachment is obsolete: true
Assignee | ||
Comment 31•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1701 at 2012-06-26 12:27:00 UTC *** And ?. Sorry about that.
Attachment #8353458 -
Flags: review?(clokep)
Assignee | ||
Comment 32•10 years ago
|
||
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 ***
Attachment #8353457 -
Attachment is obsolete: true
Attachment #8353457 -
Flags: review?(clokep)
Assignee | ||
Comment 33•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1705 at 2012-06-26 19:58:00 UTC *** Style nit: return early in case of error.
Attachment #8353462 -
Flags: review?(clokep)
Assignee | ||
Comment 34•10 years ago
|
||
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 ***
Attachment #8353458 -
Attachment is obsolete: true
Attachment #8353458 -
Flags: review?(clokep)
Comment 35•10 years ago
|
||
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. :(
Attachment #8353462 -
Flags: review?(clokep) → review-
Assignee | ||
Comment 36•10 years ago
|
||
*** 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...
Attachment #8353463 -
Flags: review?(clokep)
Assignee | ||
Comment 37•10 years ago
|
||
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 ***
Attachment #8353462 -
Attachment is obsolete: true
Comment 38•10 years ago
|
||
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.
Attachment #8353463 -
Flags: review?(clokep) → review+
Assignee | ||
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Assignee | ||
Comment 39•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1707 at 2012-06-26 23:25:00 UTC *** Move serverErrorMessage call above a potential gotDisconnected call.
Attachment #8353464 -
Flags: review?(clokep)
Assignee | ||
Comment 40•10 years ago
|
||
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 ***
Attachment #8353463 -
Attachment is obsolete: true
Comment 41•10 years ago
|
||
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 ***
Attachment #8353464 -
Flags: review?(clokep) → review+
Comment 42•10 years ago
|
||
*** Original post on bio 1540 at 2012-06-26 23:50:26 UTC *** Checked in as http://hg.instantbird.org/instantbird/rev/66d00848de1f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.2
Comment 43•10 years ago
|
||
*** Original post on bio 1540 as attmnt 1722 at 2012-06-29 20:45:00 UTC *** What do you think of these tests aleth?
Attachment #8353480 -
Flags: review?(bugzilla)
Assignee | ||
Comment 44•10 years ago
|
||
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.
Attachment #8353480 -
Flags: review?(bugzilla) → review+
Assignee | ||
Updated•10 years ago
|
Attachment #8353480 -
Flags: review?(florian)
Comment 45•10 years ago
|
||
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 ;).
Attachment #8353480 -
Flags: review?(florian) → review+
Comment 46•10 years ago
|
||
*** 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!
You need to log in
before you can comment on or make changes to this bug.
Description
•