Closed Bug 955373 Opened 7 years ago Closed 7 years ago

Blackslash not handled correctly in CTCP messages.

Categories

(Chat Core :: IRC, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: florian, Assigned: nhnt11)

References

Details

Attachments

(1 file, 4 obsolete files)

*** Original post on bio 1935 at 2013-04-16 19:51:00 UTC ***

I typed:
/me \test<enter>

The debug log shows:
Sending:
PRIVMSG #instantbird :ACTION \test

Others (including instantbot) received:
/me test
*** Original post on bio 1935 at 2013-04-16 19:57:21 UTC ***

The problem is certainly at receiving end.
Florian, it would be helpful for the solver, if you can point out parts of code where we are sending and receiving the msg.
*** Original post on bio 1935 at 2013-04-16 20:17:39 UTC ***

So, Patrick will be giving the pointers :-)
*** Original post on bio 1935 at 2013-04-16 22:47:25 UTC ***

I'd check the quoting of CTCP messages for this (maybe?):
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircCTCP.jsm#25
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#1370
http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#1335

Should we be escaping \t? Should we be unescaping \t?

We should check to see what's being sent over the network and what is being received compared to displayed.
Attached patch Patch to fix (obsolete) — Splinter Review
*** Original post on bio 1935 as attmnt 2376 at 2013-04-17 00:50:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354143 - Flags: review?(clokep)
Comment on attachment 8354143 [details] [diff] [review]
Patch to fix

*** Original change on bio 1935 attmnt 2376 at 2013-04-17 00:53:21 UTC ***

Seems to work, tested it with a few different strings.
Attachment #8354143 - Flags: review?(clokep) → review+
*** Original post on bio 1935 at 2013-04-17 00:54:31 UTC ***

Assigning (and fixing various meta data).
Assignee: nobody → nhnt11
Severity: normal → minor
Status: NEW → ASSIGNED
OS: Other → All
Hardware: x86 → All
Summary: Blackslash not handled correctly in action messages. → Blackslash not handled correctly in CTCP messages.
Whiteboard: [checkin-needed]
*** Original post on bio 1935 at 2013-04-17 09:45:52 UTC ***

Comment on attachment 8354143 [details] [diff] [review] (bio-attmnt 2376)
Patch to fix

A few questions, likely for clokep:

>   // High/CTCP level dequote: replace the quote char \134 followed by a or \134
>   // with \001 or \134, respectively. Any other character after \134 is replaced
>   // with itself.

If \134 is just \, why does the comment use the octal representation?

Writing 'a' instead of just a in the comment may also be less confusing.

>-  let dequotedCTCPMessage = message.ctcp.rawMessage.replace(/\x5C./g,
>+  let dequotedCTCPMessage = message.ctcp.rawMessage.replace(/\x5C[^\x5C]/g,

And here, \\ may be more readable than \x5C.

When seeing the hexadecimal representation in the regexp I more or less assume it's a non printable character that doesn't have a simpler representation. That may be the reason why nobody spotted the bug when initially reviewing this code.


How much effort would it take to have an xpcshell test checking that /me /foo is still /me /foo after encoding and decoding?
*** Original post on bio 1935 at 2013-04-17 10:05:44 UTC ***

(In reply to comment #7)

> How much effort would it take to have an xpcshell test checking that /me /foo
> is still /me /foo after encoding and decoding?

I meant /me \foo, not /me /foo.
*** Original post on bio 1935 at 2013-04-17 10:31:51 UTC ***

(In reply to comment #7)
> A few questions, likely for clokep:
> 
> >   // High/CTCP level dequote: replace the quote char \134 followed by a or \134
> >   // with \001 or \134, respectively. Any other character after \134 is replaced
> >   // with itself.
> 
> If \134 is just \, why does the comment use the octal representation?
Because all the specifications refer to it as \134.

> Writing 'a' instead of just a in the comment may also be less confusing.
I'd be OK changing that.

> >-  let dequotedCTCPMessage = message.ctcp.rawMessage.replace(/\x5C./g,
> >+  let dequotedCTCPMessage = message.ctcp.rawMessage.replace(/\x5C[^\x5C]/g,
> 
> And here, \\ may be more readable than \x5C.
I don't have a strong opinion about this.

> How much effort would it take to have an xpcshell test checking that /me /foo
> is still /me /foo after encoding and decoding?
It wouldn't be too bad, it'd be similar to some of the other tests we have, someone would have to sit down and write it though...CTCP quoting is something we should definitely test for though, it's complicated and error prone.
*** Original post on bio 1935 at 2013-04-17 12:08:48 UTC ***

So we're going to want to add tests for this, would you be willing to take a look at doing that? See http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/test_ircMessage.js for inspiration.
Whiteboard: [checkin-needed]
*** Original post on bio 1935 as attmnt 2405 at 2013-04-24 01:52:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354172 - Flags: review?(clokep)
*** Original post on bio 1935 at 2013-04-24 02:03:23 UTC ***

First of all, we're going to need to add the test list to http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/xpcshell.ini
Comment on attachment 8354172 [details] [diff] [review]
Add tests for handling backslashes

*** Original change on bio 1935 attmnt 2405 at 2013-04-24 02:09:33 UTC ***

I have not tested this yet, but here's some feedback.


>+function test_ctcpDequote() {
>+  let expectedOutput = [
>+    "",
>+    "test",
>+    "test",
>+    "test",
>+    "test\x5C",
>+    "\x5C\x5Ctest",
>+    "te\x5C\x5Cst",
>+    "test\x5C\x5C",
>+    "\x5C\x5Ctest",
>+    "te\x5C\x5Cst",
>+    "test\x5C\x5C\x5C",
>+    "\x01test",
>+    "te\x01st",
>+    "test\x01"
>+  ];
I find it strange that we define the input outside the function and the output inside the function.

>+  let output = input.map(function(aStr) CTCPMessage({}, aStr));
>+  for (let i = 0; i < output.length; i++)
++i, not i++

>+    do_check_eq(expectedOutput[i], output[i].param);
We should also check if output[i].command is equal to "ACTION".

>+
>+  run_next_test();
>+}
>\ No newline at end of file
Add a new line at the end of the file.

Also it should be done in the style of http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/test_splitLongMessages.js (all the code can just go into run_test).
Attachment #8354172 - Flags: review?(clokep) → review-
*** Original post on bio 1935 as attmnt 2406 at 2013-04-24 12:24:00 UTC ***

I've changed the approach of the fix in this patch. Changing the regex to /\x5C[^\x5C]/ caused pairs of backslashes to never be matched, leaving the second one in the pair to be matched with whatever follows it. For example, in "\\test", "\t" would be matched - which we don't want.

Also the unit test should be fixed up in this patch.
Attachment #8354173 - Flags: review?(clokep)
Comment on attachment 8354172 [details] [diff] [review]
Add tests for handling backslashes

*** Original change on bio 1935 attmnt 2405 at 2013-04-24 12:27:59 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354172 - Attachment is obsolete: true
Comment on attachment 8354173 [details] [diff] [review]
Updated bugfix and fixed unit test.

*** Original change on bio 1935 attmnt 2406 at 2013-04-24 12:43:55 UTC ***

>diff --git a/chat/protocols/irc/ircCTCP.jsm b/chat/protocols/irc/ircCTCP.jsm
>--- a/chat/protocols/irc/ircCTCP.jsm
>+++ b/chat/protocols/irc/ircCTCP.jsm
>@@ -3,17 +3,17 @@
>-const EXPORTED_SYMBOLS = ["ircCTCP", "ctcpBase"];
>+const EXPORTED_SYMBOLS = ["ircCTCP", "ctcpBase", "CTCPMessage"];
Revert this change.

>@@ -29,17 +29,17 @@ function CTCPMessage(aMessage, aRawCTCPM
>   let message = aMessage;
>   message.ctcp = {};
>   message.ctcp.rawMessage = aRawCTCPMessage;
> 
>   // High/CTCP level dequote: replace the quote char \134 followed by a or \134
>   // with \001 or \134, respectively. Any other character after \134 is replaced
>   // with itself.
>   let dequotedCTCPMessage = message.ctcp.rawMessage.replace(/\x5C./g,
>-    function(aStr) (aStr[1] == "a") ? "\x01" : aStr[1]);
>+    function(aStr) (aStr[1] == "a") ? "\x01" : (aStr[1] == "\x5C") ? "\x5C\x5C" : aStr[1]);
Is this over the 80 character limit? Maybe put function(aStr) on the line above it, if that works.

>diff --git a/chat/protocols/irc/test/test_ctcpDequote.js b/chat/protocols/irc/test/test_ctcpDequote.js
>@@ -0,0 +1,52 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+Components.utils.import("resource:///modules/ircCTCP.jsm");
Use a script loader instead: http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/test/test_ircMessage.js#5

>+function run_test() {
>+
Nit: remove this empty line.

>+  for (let i = 0; i < output.length; i++) {
Nit: ++i, not i++.

>+
>+  run_next_test();
>+  
Remove the blanks lines and run_next_test.

Can we add a few more tests? (The below is shown without string escaping, just with CTCP escaping)
One involving: \x5C\x5C\x5Ca -> \x5C\x01
One involving: \x5C\x5Ca -> \x5Ca
Attachment #8354173 - Flags: review?(clokep) → review-
*** Original post on bio 1935 as attmnt 2407 at 2013-04-24 14:54:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354174 - Flags: review?(clokep)
Comment on attachment 8354143 [details] [diff] [review]
Patch to fix

*** Original change on bio 1935 attmnt 2376 at 2013-04-24 14:54:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354143 - Attachment is obsolete: true
Comment on attachment 8354173 [details] [diff] [review]
Updated bugfix and fixed unit test.

*** Original change on bio 1935 attmnt 2406 at 2013-04-24 14:54:01 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354173 - Attachment is obsolete: true
Comment on attachment 8354174 [details] [diff] [review]
Updated patch in response to comments.

*** Original change on bio 1935 attmnt 2407 at 2013-04-24 18:09:40 UTC ***

>+function run_test() {
>+  let output = input.map(function(aStr) ircCTCP.CTCPMessage({}, aStr));
>+  for (let i = 0; i < output.length; i++) {
Nit: Please use ++i instead of i++.

Besides that, I just need to test it.
Attachment #8354174 - Flags: review?(clokep) → review-
*** Original post on bio 1935 as attmnt 2409 at 2013-04-24 21:25:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354176 - Flags: review?(clokep)
Comment on attachment 8354174 [details] [diff] [review]
Updated patch in response to comments.

*** Original change on bio 1935 attmnt 2407 at 2013-04-24 21:25:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354174 - Attachment is obsolete: true
Comment on attachment 8354176 [details] [diff] [review]
Fixes the ++i/i++ nit

*** Original change on bio 1935 attmnt 2409 at 2013-04-25 17:33:21 UTC ***

I got a chance to try these tests and they work. Thanks!
Attachment #8354176 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1935 at 2013-04-28 03:17:49 UTC ***

Checked in as http://hg.instantbird.org/instantbird/rev/e57aabf16a09

Congrats on fixing your first bug. :)
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
Blocks: 955399
You need to log in before you can comment on or make changes to this bug.