Repository: osa1/tiny (language: rust)

Issue:
Sending NAMES on a server tab creates new tab for each nick in server
Go to a server tab and send `NAMES` with no parameters, and watch how it opens new tabs for every nick in the response.

I believe it's happening due to the following:

In `conn.rs`:
```
// RPL_NAMREPLY: List of users in a channel
            else if n == 353 && n_params > 3 {
                let chan = &params[2];
                let chan_target = MsgTarget::Chan {
                    serv,
                    chan: ChanNameRef::new(chan),
                };

                for nick in params[3].split_whitespace() {
                    ui.add_nick(wire::drop_nick_prefix(nick), None, &chan_target);  
                }
            }
```

Here is what one RPL_NAMREPLY looks like:
```
[2021-01-16 23:21:17] DEBUG [libtiny_client/src/lib.rs:508] parsed msg: Msg { pfx: Some(Server("barjavel.freenode.net")), cmd: Reply { num: 353, params: ["trevarj_", "=", "#freenode", "Yoda ryzen taubi19_ MylesBorins bigua Harry maxalt anderson fn dfgg deni"] } }
```
Notice the last param (`param[3]`) has a ton of nicks.


Discussion / hints:
> Servers MAY only return information about the first <channel> and silently ignore the others. This seems to be an attempt to reduce possible abuse. Due to this, **clients SHOULD only query information about one channel when using the NAMES command.**
https://modern.ircdocs.horse/#names-message

😞 
I just tried this on OFTC, and I think the server doesn't show any of the described behaviors. It certainly doesn't return all names of all channels. That would be silly as it has 2,719 channels and 14,938 active users as I write this. It also doesn't ignore the message, as described in the modern.ircdocs.horse link. It just sends a bunch of seemingly random 353    RPL_NAMREPLYs then a 366    RPL_ENDOFNAMES.
We have two places where we handle 353:

- In the client: https://github.com/osa1/tiny/blob/e82046a7530e8162a785a79f53d838ba22dc6b66/libtiny_client/src/state.rs#L595-L613

  Here we just update the internal state. This does not update the GUI in any way so I think we could just leave it. Or we could just avoid creating channel state when we don't already have a state for the channel.

- At the top level: https://github.com/osa1/tiny/blob/e82046a7530e8162a785a79f53d838ba22dc6b66/tiny/src/conn.rs#L514-L525

  This is where we (perhaps unintentionally) create a channel tab on 353. We call `ui.add_nick`, which calls `apply_to_target`, which in these lines crates a new tab if a tab doesn't already exist: https://github.com/osa1/tiny/blob/e82046a7530e8162a785a79f53d838ba22dc6b66/libtiny_tui/src/tui.rs#L1133-L1138

We don't want to create a channel tab when we get a message about a channel that we're not joined (as in the case with `NAMES`), so I think (2) is where we make some changes somehow.

The last snippet above seems silly. I think we should be able to tell, when calling this method, whether we want to create a tab (if it doesn't already exist) or not. For example, for PRIVMSGs, we want to create the tab if it doesn't exist. For JOIN, same when the joined user is us (but don't create if the user is not us, though in this case if a tab doesn't already exists there is a bug somewhere else, maybe in the server, as we shouldn't get JOIN messages for channels we are not joined). For each message it should not be difficult to decide whether to create a tab or not. So I think those lines should go.

Once that's done, we simply don't create a tab in `add_nick`, which fixes this bug.
I can fix this with a boolean easily, so that the caller allows creation, then change the line to `if target_idxs.is_empty() && should_create`...this way we don't have to loop through `self.tabs` once in the caller's that can create (to see if it exists), then again in `apply_to_target` 🤷🏻 Maybe you can see a better way. 
Yeah that's what I had in mind. We don't have a lot of cases to handle (about ~20 cases) in `tiny/src/conn.rs` so shouldn't be difficult to review each one and figure out whether we want to create a tab or ignore if a tab doesn't exist.

Fix patch:
diff --git a/libtiny_tui/src/tui.rs b/libtiny_tui/src/tui.rs
index 0a4b4efc..ea6462f9 100644
--- a/libtiny_tui/src/tui.rs
+++ b/libtiny_tui/src/tui.rs
@@ -1066,7 +1066,7 @@ impl TUI {
     ////////////////////////////////////////////////////////////////////////////
     // Interfacing with tabs
 
-    fn apply_to_target<F>(&mut self, target: &MsgTarget, f: &F)
+    fn apply_to_target<F>(&mut self, target: &MsgTarget, can_create_tab: bool, f: &F)
     where
         F: Fn(&mut Tab, bool),
     {
@@ -1131,7 +1131,7 @@ impl TUI {
         }
 
         // Create server/chan/user tab when necessary
-        if target_idxs.is_empty() {
+        if target_idxs.is_empty() && can_create_tab {
             if let Some(idx) = self.maybe_create_tab(target) {
                 target_idxs.push(idx);
             }
@@ -1157,7 +1157,7 @@ impl TUI {
     }
 
     pub(crate) fn set_tab_style(&mut self, style: TabStyle, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, is_active: bool| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, is_active: bool| {
             if !is_active
                 && tab.style < style
                 && !(style == TabStyle::JoinOrPart && !tab.widget.is_showing_status())
@@ -1170,7 +1170,7 @@ impl TUI {
     /// An error message coming from Tiny, probably because of a command error
     /// etc. Those are not timestamped and not logged.
     pub(crate) fn add_client_err_msg(&mut self, msg: &str, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| {
             tab.widget.add_client_err_msg(msg);
         });
     }
@@ -1178,7 +1178,7 @@ impl TUI {
     /// A notify message coming from tiny, usually shows a response of a command
     /// e.g. "Notifications enabled".
     pub(crate) fn add_client_notify_msg(&mut self, msg: &str, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| {
             tab.widget.add_client_notify_msg(msg);
         });
     }
@@ -1186,7 +1186,7 @@ impl TUI {
     /// A message from client, usually just to indidate progress, e.g.
     /// "Connecting...". Not timestamed and not logged.
     pub(crate) fn add_client_msg(&mut self, msg: &str, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| {
             tab.widget.add_client_msg(msg);
         });
     }
@@ -1202,7 +1202,7 @@ impl TUI {
         highlight: bool,
         is_action: bool,
     ) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, true, &|tab: &mut Tab, _| {
             tab.widget
                 .add_privmsg(sender, msg, Timestamp::from(ts), highlight, is_action);
             let nick = tab.widget.get_nick();
@@ -1216,7 +1216,7 @@ impl TUI {
     /// A message without any explicit sender info. Useful for e.g. in server
     /// and debug log tabs. Timestamped and logged.
     pub fn add_msg(&mut self, msg: &str, ts: Tm, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, true, &|tab: &mut Tab, _| {
             tab.widget.add_msg(msg, Timestamp::from(ts));
         });
     }
@@ -1224,33 +1224,33 @@ impl TUI {
     /// Error messages related with the protocol - e.g. can't join a channel,
     /// nickname is in use etc. Timestamped and logged.
     pub(crate) fn add_err_msg(&mut self, msg: &str, ts: Tm, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, true, &|tab: &mut Tab, _| {
             tab.widget.add_err_msg(msg, Timestamp::from(ts));
         });
     }
 
     pub(crate) fn set_topic(&mut self, title: &str, ts: Tm, serv: &str, chan: &ChanNameRef) {
         let target = MsgTarget::Chan { serv, chan };
-        self.apply_to_target(&target, &|tab: &mut Tab, _| {
+        self.apply_to_target(&target, false, &|tab: &mut Tab, _| {
             tab.widget.show_topic(title, Timestamp::from(ts));
         });
     }
 
     pub(crate) fn clear_nicks(&mut self, serv: &str) {
         let target = MsgTarget::AllServTabs { serv };
-        self.apply_to_target(&target, &|tab: &mut Tab, _| {
+        self.apply_to_target(&target, false, &|tab: &mut Tab, _| {
             tab.widget.clear_nicks();
         });
     }
 
     pub(crate) fn add_nick(&mut self, nick: &str, ts: Option<Tm>, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| {
             tab.widget.join(nick, ts.map(Timestamp::from));
         });
     }
 
     pub(crate) fn remove_nick(&mut self, nick: &str, ts: Option<Tm>, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| {
             tab.widget.part(nick, ts.map(Timestamp::from));
         });
     }
@@ -1262,7 +1262,7 @@ impl TUI {
         ts: Tm,
         target: &MsgTarget,
     ) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| {
             tab.widget.nick(old_nick, new_nick, Timestamp::from(ts));
             // TODO: Does this actually rename the tab?
             tab.update_source(&|src: &mut MsgSource| {
@@ -1276,13 +1276,13 @@ impl TUI {
 
     pub(crate) fn set_nick(&mut self, serv: &str, new_nick: &str) {
         let target = MsgTarget::AllServTabs { serv };
-        self.apply_to_target(&target, &|tab: &mut Tab, _| {
+        self.apply_to_target(&target, false, &|tab: &mut Tab, _| {
             tab.widget.set_nick(new_nick.to_owned())
         });
     }
 
     pub(crate) fn clear(&mut self, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| tab.widget.clear());
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| tab.widget.clear());
     }
 
     pub(crate) fn toggle_ignore(&mut self, target: &MsgTarget) {
@@ -1296,11 +1296,11 @@ impl TUI {
                     }
                 }
             }
-            self.apply_to_target(target, &|tab: &mut Tab, _| {
+            self.apply_to_target(target, false, &|tab: &mut Tab, _| {
                 tab.widget.set_or_toggle_ignore(Some(!status_val));
             });
         } else {
-            self.apply_to_target(target, &|tab: &mut Tab, _| {
+            self.apply_to_target(target, false, &|tab: &mut Tab, _| {
                 tab.widget.set_or_toggle_ignore(None);
             });
         }
@@ -1319,13 +1319,13 @@ impl TUI {
     }
 
     pub(crate) fn set_notifier(&mut self, notifier: Notifier, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| {
             tab.notifier = notifier;
         });
     }
 
     pub(crate) fn show_notify_mode(&mut self, target: &MsgTarget) {
-        self.apply_to_target(target, &|tab: &mut Tab, _| {
+        self.apply_to_target(target, false, &|tab: &mut Tab, _| {
             let msg = match tab.notifier {
                 Notifier::Off => "Notifications are off",
                 Notifier::Mentions => "Notifications enabled for mentions",
diff --git a/tiny/src/conn.rs b/tiny/src/conn.rs
index d32f8d4c..ce4ac269 100644
--- a/tiny/src/conn.rs
+++ b/tiny/src/conn.rs
@@ -232,8 +232,8 @@ fn handle_irc_msg(ui: &UI, client: &dyn Client, msg: wire::Msg) {
                                 } else {
                                     MsgTarget::User { serv, nick }
                                 };
-                                ui.set_tab_style(TabStyle::Highlight, &msg_target);
                                 ui.add_privmsg(nick, &msg, ts, &msg_target, false, is_action);
+                                ui.set_tab_style(TabStyle::Highlight, &msg_target);
                             } else {
                                 // PRIVMSG not sent to us. This case can happen in a few cases:
                                 //


Test patch: