Closed
Bug 955373
Opened 10 years ago
Closed 10 years ago
Blackslash not handled correctly in CTCP messages.
Categories
(Chat Core :: IRC, defect)
Chat Core
IRC
Tracking
(Not tracked)
RESOLVED
FIXED
1.4
People
(Reporter: florian, Assigned: nhnt11)
References
Details
Attachments
(1 file, 4 obsolete files)
3.60 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
*** 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
Comment 1•10 years ago
|
||
*** 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.
Comment 2•10 years ago
|
||
*** Original post on bio 1935 at 2013-04-16 20:17:39 UTC *** So, Patrick will be giving the pointers :-)
Comment 3•10 years ago
|
||
*** 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.
Assignee | ||
Comment 4•10 years ago
|
||
*** 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 5•10 years ago
|
||
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+
Comment 6•10 years ago
|
||
*** 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]
Reporter | ||
Comment 7•10 years ago
|
||
*** 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?
Reporter | ||
Comment 8•10 years ago
|
||
*** 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.
Comment 9•10 years ago
|
||
*** 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.
Comment 10•10 years ago
|
||
*** 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]
Assignee | ||
Comment 11•10 years ago
|
||
*** 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)
Comment 12•10 years ago
|
||
*** 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 13•10 years ago
|
||
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-
Assignee | ||
Comment 14•10 years ago
|
||
*** 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 15•10 years ago
|
||
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 16•10 years ago
|
||
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-
Assignee | ||
Comment 17•10 years ago
|
||
*** 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)
Assignee | ||
Comment 18•10 years ago
|
||
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
Assignee | ||
Comment 19•10 years ago
|
||
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 20•10 years ago
|
||
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-
Assignee | ||
Comment 21•10 years ago
|
||
*** 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)
Assignee | ||
Comment 22•10 years ago
|
||
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 23•10 years ago
|
||
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+
Updated•10 years ago
|
Whiteboard: [checkin-needed]
Comment 24•10 years ago
|
||
*** 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: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.4
You need to log in
before you can comment on or make changes to this bug.
Description
•