Closed Bug 955405 Opened 8 years ago Closed 8 years ago

CTCP quoting is broken

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

Details

Attachments

(1 file, 1 obsolete file)

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

*** Due to BzAPI limitations, the initial description is in comment 1 ***
*** 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.
Attachment #8354212 - Flags: review?(aleth)
Attached patch Patch with testsSplinter Review
*** 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.
Attachment #8354218 - Flags: review?(aleth)
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 ***
Attachment #8354212 - Attachment is obsolete: true
Attachment #8354212 - Flags: review?(aleth)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
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))
Attachment #8354218 - Flags: review-
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.
Attachment #8354218 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** 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 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+.
Attachment #8354218 - Flags: review- → review+
*** Original post on bio 1968 at 2013-05-24 22:42:37 UTC ***

http://hg.instantbird.org/instantbird/rev/079a63e7c003
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.