Closed Bug 955742 Opened 6 years ago Closed 5 years ago

Joining multiple chats at once opens spurious MUC

Categories

(Chat Core :: IRC, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: aleth)

Details

Attachments

(1 file, 1 obsolete file)

*** Original post on bio 2289 at 2013-12-21 19:04:00 UTC ***

An additional MUC is opened whose name is the comma-separated parameter passed to Join Chat.
*** Original post on bio 2289 at 2013-12-26 17:24:48 UTC ***

This isn't actually a regression. It only happens when using the Join Chat dialog, and it happens because that dialog does not support the comma-separated syntax for multiple MUCs at once. The fact that it works at all for IRC is accidental (we send "JOIN #channel1,#channel2" and irc.mozilla.org copes). It's only recently become visible because we now open a "#channel1,#channel2" tab.

I'm tempted not to fix this in the hope that nhnt11 will make Join Chat go away sooner rather than later anyway.
Summary: [regression] Joining multiple chats at once opens spurious MUC → Joining multiple chats at once opens spurious MUC
I don't think we should fix this, the syntax that we use for e.g. autojoin just doesn't make sense in that dialog (since it has a separate password and channel name field).
so why are we opening a broken tab? If invalid syntax is entered by the user, could we reject it or put an error message in the console instead of doing something obviously wrong?
Attached patch joinsanitize.diff (obsolete) — Splinter Review
This stops us from opening broken channels. Left for the future, a Join Chat dialog replacement that doesn't fail silently etc.
Assignee: nobody → aleth
Status: NEW → ASSIGNED
Attachment #8477416 - Flags: review?(clokep)
Comment on attachment 8477416 [details] [diff] [review]
joinsanitize.diff

Review of attachment 8477416 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/irc/irc.js
@@ +1442,5 @@
>    // aComponents implements prplIChatRoomFieldValues.
>    joinChat: function(aComponents) {
>      let channel = aComponents.getValue("channel");
> +    // Mildly sanitize input.
> +    channel = channel.trimLeft().split(",")[0].split(" ")[0];

Does this break having multiple things in auto-join or does the UI do a split(',') somewhere?

What is the split on space for?

Why trimLeft() instead of trim()?

@@ +1448,5 @@
>        this.ERROR("joinChat called without a channel name.");
>        return null;
>      }
>  
> +

Nit: No reason for a double empty line here.
> Does this break having multiple things in auto-join or does the UI do a
> split(',') somewhere?

This has nothing to do with autojoin as joinChat does not takes more than one channel parameter. Our funky autojoin/join command syntax is just the reason people have tried to use the Join Chat dialog to join multiple channels at once, which is not supported.

> What is the split on space for?

People trying to put passwords in the channel field.

> Why trimLeft() instead of trim()?

The split on space takes care of trimRight.
Attachment #8477416 - Attachment is obsolete: true
Attachment #8477416 - Flags: review?(clokep)
Attachment #8478995 - Flags: review?(clokep)
Comment on attachment 8478995 [details] [diff] [review]
joinsanitize.diff 2

Review of attachment 8478995 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with an expanded comment describing what / why this is being done.

::: chat/protocols/irc/irc.js
@@ +1442,5 @@
>    // aComponents implements prplIChatRoomFieldValues.
>    joinChat: function(aComponents) {
>      let channel = aComponents.getValue("channel");
> +    // Mildly sanitize input.
> +    channel = channel.trimLeft().split(",")[0].split(" ")[0];

Expand the comment saying what this is actually doing.
Attachment #8478995 - Flags: review?(clokep) → review+
Doh, I guess I wanted a better comment. Oh well, I've committed this. Don't worry about it!

https://hg.mozilla.org/comm-central/rev/d96abff53709
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.