Sending NAMES on a server tab creates new tab for each nick in server

[trevarj]

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.

[trevarj]

Servers MAY only return information about the first 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

😞

[osa1]

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.

[osa1]

We have two places where we handle 353:

• In the client:

  tiny/libtiny_client/src/state.rs

  Lines 595 to 613 in e82046a

  	// RPL_NAMREPLY: Set users in a channel
  	Reply { num: 353, params } => {
  	let chan = ChanNameRef::new(&params[2]);
  	match utils::find_idx(&self.chans, |c| &c.name == chan) {
  	None => self.chans.push(Chan::with_nicks(
  	chan.to_owned(),
  	params[3]
  	.split_whitespace()
  	.map(|s| wire::drop_nick_prefix(s).to_owned())
  	.collect(),
  	)),
  	Some(idx) => {
  	let nick_set = &mut self.chans[idx].nicks;
  	for nick in params[3].split_whitespace() {
  	nick_set.insert(wire::drop_nick_prefix(nick).to_owned());
  	}
  	}
  	}
  	}

  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:

  tiny/tiny/src/conn.rs

  Lines 514 to 525 in e82046a

  	// 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);
  	}
  	}

  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:

  tiny/libtiny_tui/src/tui.rs

  Lines 1133 to 1138 in e82046a

  	// Create server/chan/user tab when necessary
  	if target_idxs.is_empty() {
  	if let Some(idx) = self.maybe_create_tab(target) {
  	target_idxs.push(idx);
  	}
  	}

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.

[osa1]

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.

[trevarj]

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.

[osa1]

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.

[trevarj]

I thought it was as simple as this: master...trevarj:names-fix

but it looks like there's a bug where it won't highlight a new tab for a user privmsg...No time to debug right now

Edit: solved in PR