954455 - IRC sends unrecognized commands to active conversation

Status: RESOLVED FIXED

(Chat Core :: General, defect)

Description

[Instantbot]

*** Original post on bio 1020 by Matt Lewandowsky (lewellyn) <matt AT greenviolet.net> at 2011-09-06 18:12:00 UTC ***

Instead of warning the user that a '/command' that they used is unrecognized, Instantbird currently sends the user's input to the active conversation.

At best, this is an annoyance (/ignore someuser). At worst, it could be compromising (/nickserv identify mymainnick mypassword).

I see two possible scenarios for correcting this:

1) Capture any unrecognized commands and respond to the user that they are invalid.

2) Send any unrecognized commands raw to the server. This will let the server catch anything which can be dealt with server-side, while still (correctly) reporting that unrecognized commands (such as /ignore) are unknown.

The latter is theoretically the "better" solution as it prevents needing to "know" which commands are supported by the network (such as Freenode's /remove or many networks' /nickserv, /chanserv, et al.).

In either case, there will need to be "double-slash" and/or "slash-space" prefixing support to get the current behavior. This may be better suited for a dependent bug. (Note that, in practice, most IRC users who start lines with a / simply prefix it with a space to avoid problems.)

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 1020 at 2011-09-06 18:35:08 UTC ***

(In reply to comment #0)
> 2) Send any unrecognized commands raw to the server. This will let the server
> catch anything which can be dealt with server-side, while still (correctly)
> reporting that unrecognized commands (such as /ignore) are unknown.
This is difficult though, how do I know that "/foo bar foobar" is meant to be sent as "FOO bar :foobar" instead of "FOO :bar foobar"? You can take a guess, but I think it's a more confusing UX.

> The latter is theoretically the "better" solution as it prevents needing to
> "know" which commands are supported by the network
I disagree and I think #1 is a better solution, we have the /quote command for sending raw data to the server.

> or many networks' /nickserv, /chanserv, et al.).
These aren't really true though the /nickserv command sends PRIVMSG nickserv, so it's not really passing it directly through.

> In either case, there will need to be "double-slash" and/or "slash-space"
> prefixing support to get the current behavior. This may be better suited for a
> dependent bug. (Note that, in practice, most IRC users who start lines with a /
> simply prefix it with a space to avoid problems.)
I think " /" should cause a command to print to the conversation.

Comment 2

[Florian Quèze [:florian]]

*** Original post on bio 1020 at 2011-09-06 20:34:37 UTC ***

(In reply to comment #0)

> 1) Capture any unrecognized commands and respond to the user that they are
> invalid.
We used to throw when an unrecognized command was entered. Unfortunately there was no error message displayed in the conversation, it was only in the error console.
Left-over: http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/public/purpleIProtocol.idl#117

> 2) Send any unrecognized commands raw to the server.
If possible it would seem better to me, but per comment 1, it doesn't seem possible :-/.

> In either case, there will need to be "double-slash" and/or "slash-space"
> prefixing support to get the current behavior.

I think the /say command does that.

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 1020 at 2011-09-13 12:17:49 UTC ***

We need our command service to abide by http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/public/purpleIProtocol.idl#117 :

> // OPT_PROTO_SLASH_COMMANDS_NATIVE  Indicates that slash commands
> // are native to this protocol.
> // Used as a hint that unknown commands should not be sent as messages.
> readonly attribute boolean slashCommandsNative;

It would seem somewhere near http://lxr.instantbird.org/instantbird/source/purple/purplexpcom/src/imCommands.js#228 we would need to branch if (aConversation.account.protocol.slashCommandsNative) and add a default handler that just sends the command (or something along those lines).

Comment 4

[Florian Quèze [:florian]]

Attachment: Example

*** Original post on bio 1020 as attmnt 815 at 2011-09-13 13:25:00 UTC ***

I would prefer that we handle it in conversation.xml (see the attachment for an example of where the test could go).

Comment 5

[Patrick Cloke [:clokep]]

Attachment: version 1.0

*** Original post on bio 1020 as attmnt 819 at 2011-09-14 00:23:00 UTC ***

When an unknown command is given this displays a system message of the form "foo is an unknown command." (where the user typed in "/foo" or perhaps "/foo bar").

This leaves the command in the input box, I'm not sure if this is desired or not. I feel like it should be left so people can fix their mistake (although you can always get the previous message with <ctrl>+z). It could also confuse uses and seem like an error, although it also reinforces that the message was not sent.

Comment 6

[Patrick Cloke [:clokep]]

Attachment: version 1.1

*** Original post on bio 1020 as attmnt 820 at 2011-09-14 00:52:00 UTC ***

Meets some comments from IRC. flo wanted a more specific message (one that didn't say "unknown" and that mentioned using /help).

I also found a bug in my regexp, I had .+ instead of .* after the command.

This also removes the sendTyping(0).

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8352562 [details] [diff] [review]
version 1.0

*** Original change on bio 1020 attmnt 819 at 2011-09-14 00:52:48 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 8

[Florian Quèze [:florian]]

Attachment: Patch v1.1+

*** Original post on bio 1020 as attmnt 821 at 2011-09-14 11:03:00 UTC ***

I tinkered with this a bit while reviewing, and I'm still not fully satisfied, especially of the handling of "/" and "/ ". I'm also wondering if /say should become a real command rather than a workaround in conversation.xml

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8352558 [details] [diff] [review]
Example

*** Original change on bio 1020 attmnt 815 at 2011-09-14 11:03:44 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8352563 [details] [diff] [review]
version 1.1

*** Original change on bio 1020 attmnt 820 at 2011-09-14 23:09:43 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 11

[Patrick Cloke [:clokep]]

Attachment: Patch v1.2

*** Original post on bio 1020 as attmnt 825 at 2011-09-16 01:18:00 UTC ***

This meets some (all?) of the changes that were discussed on IRC to ignore non-sense input (such as "/" or "/ " or "/say" or "/say "). It might need a bit more work, but I think we're converging on a solution here.

Note that flo and I realized you can't implement /say as a normal command since it depends on the conversation binding and not just on the backend. (Although this causes there to be no help information on say, which I don't like. Perhaps we could have it registered somewhere else, instead of as part of the commands service.)

Comment 12

[Patrick Cloke [:clokep]]

Comment on attachment 8352564 [details] [diff] [review]
Patch v1.1+

*** Original change on bio 1020 attmnt 821 at 2011-09-16 01:18:52 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[Florian Quèze [:florian]]

Comment on attachment 8352568 [details] [diff] [review]
Patch v1.2

*** Original change on bio 1020 attmnt 825 at 2011-09-16 10:35:40 UTC ***

>diff --git a/instantbird/content/conversation.xml b/instantbird/content/conversation.xml

>-        // the /say command is used to bypass command processing
>-        var msg = aMsg.match(/^\/say (.*)/);
>-        if (!msg) {
>-          if (Services.cmd.executeCommand(aMsg, this._conv.target)) {
>+        if (aMsg[0] == "/") {
>+          // The /say command is used to bypass command processing.
>+          // "/say" or "/say " should be ignored, as should "/" and "/ ".
>+          if (aMsg.match(/^\/(?:say)? ?$/)) {

Here the code treats /say and / as if they were identical.

>+            this.resetInput();
>+            return;
>+          }
>+          else if (aMsg.match(/^\/say .*/))
>+            aMsg = aMsg.slice(5);

But here only /say is handled.

Comment 14

[Patrick Cloke [:clokep]]

Attachment: Patch v1.3

*** Original post on bio 1020 as attmnt 828 at 2011-09-16 10:37:00 UTC ***

Treats /say and / identically.

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8352568 [details] [diff] [review]
Patch v1.2

*** Original change on bio 1020 attmnt 825 at 2011-09-16 10:37:29 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 16

[Florian Quèze [:florian]]

Comment on attachment 8352571 [details] [diff] [review]
Patch v1.3

*** Original change on bio 1020 attmnt 828 at 2011-09-16 11:06:08 UTC ***

Looks good, thanks for figuring this out!

Before pushing it I'll just add in a comment that /say can be shortened to just /.

Comment 17

[Patrick Cloke [:clokep]]

*** Original post on bio 1020 at 2011-09-16 11:50:35 UTC ***

Checked-in as http://hg.instantbird.org/instantbird/rev/16877232760e

Comment 18

[Florian Quèze [:florian]]

Attachment: Follow-up to restore the previous /me behavior

*** Original post on bio 1020 as attmnt 842 at 2011-09-22 10:05:00 UTC ***

The patch as checked-in breaks /me on XMPP.

/me handling used to rely on unrecognized commands to be sent. With the changes made here, it is no longer the case for prpls with the slashCommandsNative flag. Those prpls are IRC and XMPP.
/me still works on IRC because the IRC plugin registers a /me command.

There are two possible fix:
 - add a special case in conversation.xml for the /me command, to restore the previous behavior. Given that we already special case /me in that file for typing notifications, and that it's a trivial fix, I think this is the best thing to do. (The attached patch does this).
 - the alternative fix is to register a /me command in the XMPP plugin. That would be more consistent with what the IRC plugin does, but I think it's overkill.

Comment 19

[Patrick Cloke [:clokep]]

Comment on attachment 8352585 [details] [diff] [review]
Follow-up to restore the previous /me behavior

*** Original change on bio 1020 attmnt 842 at 2011-09-22 21:03:27 UTC ***

Since /me is already special cased this looks good. I dislike how /me is just sent for XMPP, but that's not an issue with this patch.

Comment 20

[Florian Quèze [:florian]]

*** Original post on bio 1020 at 2011-09-22 22:50:27 UTC ***

Comment on attachment 8352585 [details] [diff] [review] (bio-attmnt 842)
Follow-up to restore the previous /me behavior

https://hg.instantbird.org/instantbird/rev/17ade62e17b3