955405 - CTCP quoting is broken

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 1968 at 2013-05-22 19:51:00 UTC ***

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

Comment 1

[Patrick Cloke [:clokep]]

Attachment: Don't generate the regexp automatically, which requires escaping twice

*** Original post on bio 1968 as attmnt 2445 at 2013-05-22 19:51:00 UTC ***

let input = "this \\is \\\\a \\\\\\cool \\\\\\\\string also this: \x01";
print(input)

// The current Instantbird code.
const highQuote = {"\\": "\\", "\x01": "a"};
const highRegex = new RegExp(Object.keys(highQuote).join("|"), "g");

print(highRegex)

let output = input.replace(highRegex, function(aChar) "\x5C" + highQuote[aChar]);
print(output)

// New code.
const pattern = /\\|\x01/g;
print(pattern)

let outputNew = input.replace(pattern, function(aChar) "\x5C" + highQuote[aChar]);
print(outputNew)

/*
 * Output below:
3: this \is \\a \\\cool \\\\string also this: 
9: /\|/g
12: this \is \\a \\\cool \\\\string also this: 
16: /\\|\x01/g
19: this \\is \\\\a \\\\\\cool \\\\\\\\string also this: \a
 */

We should add tests to this.

Comment 2

[Patrick Cloke [:clokep]]

Attachment: Patch with tests

*** Original post on bio 1968 as attmnt 2451 at 2013-05-23 03:11:00 UTC ***

I added tests and simplified the code a bit.

Comment 3

[Patrick Cloke [:clokep]]

Comment on attachment 8354212 [details] [diff] [review]
Don't generate the regexp automatically, which requires escaping twice

*** Original change on bio 1968 attmnt 2445 at 2013-05-23 03:11:31 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 4

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8354218 [details] [diff] [review]
Patch with tests

*** Original change on bio 1968 attmnt 2451 at 2013-05-23 13:19:58 UTC ***

+  "\\\\\\test",
+  "te\\\\\\st",
+  "test\\\\\\",

These are probably unnecessary.

Additionally I think we should have tests for multiple \x01's in a row, like "\x01\x01test" (this goes for dequoting as well, bug 955399 (bio 1962))

Comment 5

[aleth [:aleth]]

Comment on attachment 8354218 [details] [diff] [review]
Patch with tests

*** Original change on bio 1968 attmnt 2451 at 2013-05-24 13:30:46 UTC ***

Quite a subtle double-escaping bug! Fix looks fine to me.

I don't think those additional tests are a problem, even if they are maybe unnecessary. I don't have a strong opinion on whether multiple /x01 tests are needed here.

Comment 6

[Patrick Cloke [:clokep]]

*** Original post on bio 1968 at 2013-05-24 21:37:47 UTC ***

(In reply to comment #2)
> Comment on attachment 8354218 [details] [diff] [review] (bio-attmnt 2451) [details]
> Patch with tests
> 
> +  "\\\\\\test",
> +  "te\\\\\\st",
> +  "test\\\\\\",
> 
> These are probably unnecessary.
I like having these tests here for completeness.

> Additionally I think we should have tests for multiple \x01's in a row, like
> "\x01\x01test" (this goes for dequoting as well, bug 955399 (bio 1962))
I'm not really sure we need these, I like the multiple backslash tests because of the escaping issues with backslashes. If you insist we can add them.

Comment 7

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8354218 [details] [diff] [review]
Patch with tests

*** Original change on bio 1968 attmnt 2451 at 2013-05-24 22:31:51 UTC ***

Okay. I can't see any other problems so r+.

Comment 8

[Patrick Cloke [:clokep]]

*** Original post on bio 1968 at 2013-05-24 22:42:37 UTC ***

http://hg.instantbird.org/instantbird/rev/079a63e7c003