Closed Bug 955045 Opened 9 years ago Closed 9 years ago

IRC CTCP messages break if there is a line break

Categories

(Chat Core :: IRC, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 1615 at 2012-08-03 10:50:00 UTC ***

After attempting to send:
/me feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.)
Source File: http://hg.instantbird.org/instantbird/raw-file/83739a138f9d/purple/libpurple/win32/win32dep.c Line: 89 Source Code: wpurple: wpurple_find_and_loadproc

I got a warning back:
Warning: Unhandled IRC message: :concrete.mozilla.org 421 clokep Source :Unknown command
Source File: jar:file:///C:/Program%20Files%20(x86)/Instantbird/omni.ja!/components/irc.js
Line: 485

And people on the other end received:
6:48:28 AM - clokep: ACTION feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.)

The actual messages being sent is:
PRIVMSG clokep :ACTION feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.)
Source File: http://hg.instantbird.org/instantbird/raw-file/83739a138f9d/purple/libpurple/win32/win32dep.c Line: 89 Source Code: wpurple: wpurple_find_and_loadproc 

And we receive:
{"rawMessage":":clokep!Instantbir@moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com PRIVMSG clokep :\u0001ACTION feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.)","command":"PRIVMSG","params":["clokep","\u0001ACTION feels like this is a new scary error: Error: Could not load: dnssd.dll (The specified module could not be found.)"],"nickname":"clokep","user":"Instantbir","host":"moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com","source":"Instantbir@moz-69FB3955.c3-0.smr-ubr1.sbo-smr.ma.cable.rcn.com"}
Summary: IRC ACTION (CTCP?) messages break with a line break → IRC CTCP messages break if there is a line break
Attached patch Patch v1 (obsolete) — Splinter Review
*** Original post on bio 1615 as attmnt 1824 at 2012-08-21 23:49:00 UTC ***

This removes the noNewlines flag for accounts and implements it directly in IRC (which is the only account type that used it). This is done in the sendMsg function.

This also handles CTCP quoting better, when receiving messages I inlined the functions since they're only called once. When sending messages we now actually quote the outgoing messages, this allows us to send \r or \n in outgoing messages.

I'd like aleth to review the IRC bits and flo to review the purplexpcom bits (at least, it'd be great if you could both look at both!)
Attachment #8353583 - Flags: review?(bugzilla)
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Comment on attachment 8353583 [details] [diff] [review]
Patch v1

*** Original change on bio 1615 attmnt 1824 at 2012-08-21 23:50:08 UTC ***

Also, for reference: http://www.irchelp.org/irchelp/rfc/ctcpspec.html
Attachment #8353583 - Flags: review?(florian)
Comment on attachment 8353583 [details] [diff] [review]
Patch v1

*** Original change on bio 1615 attmnt 1824 at 2012-08-22 21:23:19 UTC ***

Looks good - I can't see anything wrong with the code, just some nits/questions:

Looking at the original STR, would we want an a action message with line breaks to be split up into a set of /me messages for sending? This patch doesn't do this - only the first line will be an action message.

irc.js:sendRawMessage:
 // TODO This should escape any characters that can't be used in IRC (e.g.
 // \001, \r\n).
At least the latter example is obsolete now, as you now simply send multiple PRIVMSG when its not a CTCP message, right?
And I'm not sure you'd want to escape \001 seeing as you have added that in sendCTCPmessage?
(i.e. I am a bit confused about this TODO).

>    // High/CTCP level quoting, replace \123 or \001 with \123\123 or \123a,
>    // respectively
Are you sure you don't mean \134? ;)

>const highQuote = {"\x5C": "a", "\x01": "a"};
The "a" entries are not used here, and I guess you are only including them in parallel to lowQuote for legibility, however I think the first one is wrong anyway.

>  // The raw CTCP message is in the last parameter of the IRC message. Low level
Please start a new line before "Low level", otherwise it is harder to spot. I also think a colon after "Low level quote" would be better than a comma (similarly for the "High level quote" comment).

>  const replacements = {"0": "\0", "n": "\n", "r": "\r", "\x10": "\x10"};
Maybe call this inverseLowQuote for consistency with the encoder?
Attachment #8353583 - Flags: review?(bugzilla) → review-
*** Original post on bio 1615 at 2012-08-27 02:53:23 UTC ***

(In reply to comment #3)
> Looking at the original STR, would we want an a action message with line breaks
> to be split up into a set of /me messages for sending? This patch doesn't do
> this - only the first line will be an action message.
With this patch the line breaks are escaped and sent as part of the action statement. I don't think it would make sense (in this case) to send them as separate ones, you would then get:
"clokep is tired
clokep and is hungry"
instead of:
"clokep is tired
and hungry"

(Why you'd put a line break, IDK, but whatever...)

Normal messages sent from the UI should get broken into multiple messages on a line break, however.

> irc.js:sendRawMessage:
>  // TODO This should escape any characters that can't be used in IRC (e.g.
>  // \001, \r\n).
> At least the latter example is obsolete now, as you now simply send multiple
> PRIVMSG when its not a CTCP message, right?
> And I'm not sure you'd want to escape \001 seeing as you have added that in
> sendCTCPmessage?
> (i.e. I am a bit confused about this TODO).
So...we should really be escaping all the characters here according to the CTCP spec (characters get escaped even when not instead of CTCP tags (\001)). I'm going to (figure out) how to do this better so we can escape all the messages in one place, once and not do it in multiple places. This probably means I need to rework some code though since we can't have the sendCTCPMessage add \001 characters and then try to escape the...

> >    // High/CTCP level quoting, replace \123 or \001 with \123\123 or \123a,
> >    // respectively
> Are you sure you don't mean \134? ;)
Thanks. :)

> >const highQuote = {"\x5C": "a", "\x01": "a"};
> The "a" entries are not used here, and I guess you are only including them in
> parallel to lowQuote for legibility, however I think the first one is wrong
> anyway.
\x5C should be replaced by \x5C\x5C and \x01 should become \x5Ca, so this should be done identically to the lowQuote.

> >  // The raw CTCP message is in the last parameter of the IRC message. Low level
> Please start a new line before "Low level", otherwise it is harder to spot. I
> also think a colon after "Low level quote" would be better than a comma
> (similarly for the "High level quote" comment).
Yeah, I added an extra variable too just to be explicit.

> >  const replacements = {"0": "\0", "n": "\n", "r": "\r", "\x10": "\x10"};
> Maybe call this inverseLowQuote for consistency with the encoder?
I called it lowDequote (dequote is what the spec calls the opposite of quote).

This needs a bit of work and thought so I'll come up with a new patch eventually.
Comment on attachment 8353583 [details] [diff] [review]
Patch v1

*** Original change on bio 1615 attmnt 1824 at 2012-08-27 12:43:45 UTC ***

This patch needs some more work, canceling the review flag for flo.
Attachment #8353583 - Flags: review?(florian)
Attached patch Patch v2Splinter Review
*** Original post on bio 1615 as attmnt 1870 at 2012-08-29 03:50:00 UTC ***

Takes into account the feedback offered. This follows the CTCP spec more closely: it does low level (de)quoting for every message and only does the high (de)quoting inside of CTCP messages. Gets rid of a TODO comment too. :)
Attachment #8353628 - Flags: review?(bugzilla)
Comment on attachment 8353583 [details] [diff] [review]
Patch v1

*** Original change on bio 1615 attmnt 1824 at 2012-08-29 03:50:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353583 - Attachment is obsolete: true
Comment on attachment 8353628 [details] [diff] [review]
Patch v2

*** Original change on bio 1615 attmnt 1870 at 2012-08-29 13:56:11 UTC ***

Looks good :)
Attachment #8353628 - Flags: review?(bugzilla) → review+
Attachment #8353628 - Flags: review?(florian)
Comment on attachment 8353628 [details] [diff] [review]
Patch v2

*** Original change on bio 1615 attmnt 1870 at 2012-08-29 22:31:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8353628 - Flags: review?(florian) → review+
*** Original post on bio 1615 at 2012-08-30 02:49:00 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/963c245d7f0d
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3
*** Original post on bio 1615 at 2012-09-01 12:17:43 UTC ***

Follow up to show outgoing messages as we actually send them: http://hg.instantbird.org/instantbird/rev/824b4152cea8
Depends on: 955690
You need to log in before you can comment on or make changes to this bug.