fix(rooms): harden chat-room parsing and TUI state from review findings
Adversarial review of the chat-rooms feature surfaced seven confirmed issues; all fixed with regression tests: lib (DoS hardening — a malicious/MITM server could send a tiny frame with a ~4-billion element count): - room_list: bound the name/count vector loops by remaining payload. - join_room: bound the member-list loop the same way. TUI (soulseek-rs/src/models/rooms.rs, ui): - a message for a room we already left no longer resurrects its tab (route via open_index, not ensure_open). - closing a tab now clears the unread badge on the room we land on. - a room-list refresh keeps the highlight on the same room across a re-sort (track selection by name, not index). - Esc in the room list clears a lingering filter before closing, matching the title's 'Esc: clear' hint. - chat log renders one row per line so the newest message is never clipped by wrapping.
This commit is contained in:
@@ -19,6 +19,11 @@ impl MessageHandler<ServerMessage> for JoinRoomHandler {
|
|||||||
let user_count = message.read_int32();
|
let user_count = message.read_int32();
|
||||||
let mut users = Vec::new();
|
let mut users = Vec::new();
|
||||||
for _ in 0..user_count {
|
for _ in 0..user_count {
|
||||||
|
// Guard against a hostile user_count outrunning the payload; each
|
||||||
|
// username is a length-prefixed string (>= 4 bytes).
|
||||||
|
if message.get_pointer() + 4 > message.get_size() {
|
||||||
|
break;
|
||||||
|
}
|
||||||
users.push(message.read_string());
|
users.push(message.read_string());
|
||||||
}
|
}
|
||||||
let _ = sender.send(ServerMessage::RoomJoined { room, users });
|
let _ = sender.send(ServerMessage::RoomJoined { room, users });
|
||||||
@@ -29,6 +34,26 @@ impl MessageHandler<ServerMessage> for JoinRoomHandler {
|
|||||||
mod tests {
|
mod tests {
|
||||||
use super::*;
|
use super::*;
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn hostile_user_count_does_not_hang() {
|
||||||
|
// room="" then user_count=u32::MAX with no usernames: must return
|
||||||
|
// promptly instead of looping ~4 billion times.
|
||||||
|
let (tx, rx) = std::sync::mpsc::channel();
|
||||||
|
let mut message = Message::new();
|
||||||
|
message.write_raw_bytes(vec![0u8; 8]);
|
||||||
|
message.write_string("");
|
||||||
|
message.write_int32(u32::MAX);
|
||||||
|
message.set_pointer(8);
|
||||||
|
|
||||||
|
JoinRoomHandler.handle(&mut message, tx);
|
||||||
|
match rx.try_recv() {
|
||||||
|
Ok(ServerMessage::RoomJoined { users, .. }) => {
|
||||||
|
assert!(users.is_empty());
|
||||||
|
}
|
||||||
|
other => panic!("unexpected: {other:?}"),
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn forwards_room_and_member_list() {
|
fn forwards_room_and_member_list() {
|
||||||
let (tx, rx) = std::sync::mpsc::channel();
|
let (tx, rx) = std::sync::mpsc::channel();
|
||||||
|
|||||||
@@ -27,11 +27,20 @@ pub fn parse_room_list(message: &mut Message) -> Vec<RoomInfo> {
|
|||||||
let name_count = message.read_int32();
|
let name_count = message.read_int32();
|
||||||
let mut names = Vec::new();
|
let mut names = Vec::new();
|
||||||
for _ in 0..name_count {
|
for _ in 0..name_count {
|
||||||
|
// Stop once the payload can't hold another field: a bogus (possibly
|
||||||
|
// hostile) count must not spin us into an OOM allocation loop. A
|
||||||
|
// 4-byte floor also avoids read_int32 stalling on a 1-3 byte tail.
|
||||||
|
if message.get_pointer() + 4 > message.get_size() {
|
||||||
|
break;
|
||||||
|
}
|
||||||
names.push(message.read_string());
|
names.push(message.read_string());
|
||||||
}
|
}
|
||||||
let count_count = message.read_int32();
|
let count_count = message.read_int32();
|
||||||
let mut counts = Vec::new();
|
let mut counts = Vec::new();
|
||||||
for _ in 0..count_count {
|
for _ in 0..count_count {
|
||||||
|
if message.get_pointer() + 4 > message.get_size() {
|
||||||
|
break;
|
||||||
|
}
|
||||||
counts.push(message.read_int32());
|
counts.push(message.read_int32());
|
||||||
}
|
}
|
||||||
names
|
names
|
||||||
@@ -90,6 +99,17 @@ mod tests {
|
|||||||
assert!(parse_room_list(&mut message).is_empty());
|
assert!(parse_room_list(&mut message).is_empty());
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn hostile_counts_do_not_hang_or_overallocate() {
|
||||||
|
// A tiny frame claiming ~4 billion names/counts must return promptly
|
||||||
|
// (bounded by the payload) rather than looping into an OOM.
|
||||||
|
let mut message = framed(|m| {
|
||||||
|
m.write_int32(u32::MAX);
|
||||||
|
});
|
||||||
|
let rooms = parse_room_list(&mut message);
|
||||||
|
assert!(rooms.is_empty());
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn handler_forwards_parsed_rooms() {
|
fn handler_forwards_parsed_rooms() {
|
||||||
let (tx, rx) = std::sync::mpsc::channel();
|
let (tx, rx) = std::sync::mpsc::channel();
|
||||||
|
|||||||
@@ -155,6 +155,9 @@ impl RoomsState {
|
|||||||
}
|
}
|
||||||
if self.open.is_empty() {
|
if self.open.is_empty() {
|
||||||
self.view = RoomsView::List;
|
self.view = RoomsView::List;
|
||||||
|
} else {
|
||||||
|
// The tab we land on is now being viewed; clear its stale badge.
|
||||||
|
self.mark_active_read();
|
||||||
}
|
}
|
||||||
Some(room.name)
|
Some(room.name)
|
||||||
}
|
}
|
||||||
@@ -192,9 +195,18 @@ impl RoomsState {
|
|||||||
pub fn apply_event(&mut self, event: RoomEvent, viewing: Option<&str>) {
|
pub fn apply_event(&mut self, event: RoomEvent, viewing: Option<&str>) {
|
||||||
match event {
|
match event {
|
||||||
RoomEvent::List(rooms) => {
|
RoomEvent::List(rooms) => {
|
||||||
|
// Keep the highlight on the same room across a re-sort, since
|
||||||
|
// list_selected is a positional index into the sorted list.
|
||||||
|
let selected_name = self.selected_room_name();
|
||||||
self.available = rooms;
|
self.available = rooms;
|
||||||
let max = self.filtered_rooms().len().saturating_sub(1);
|
let filtered = self.filtered_rooms();
|
||||||
self.list_selected = self.list_selected.min(max);
|
self.list_selected = selected_name
|
||||||
|
.and_then(|name| {
|
||||||
|
filtered.iter().position(|r| r.name == name)
|
||||||
|
})
|
||||||
|
.unwrap_or_else(|| {
|
||||||
|
self.list_selected.min(filtered.len().saturating_sub(1))
|
||||||
|
});
|
||||||
}
|
}
|
||||||
RoomEvent::Joined { room, users } => {
|
RoomEvent::Joined { room, users } => {
|
||||||
let idx = self.ensure_open(&room);
|
let idx = self.ensure_open(&room);
|
||||||
@@ -215,12 +227,17 @@ impl RoomsState {
|
|||||||
username,
|
username,
|
||||||
message,
|
message,
|
||||||
} => {
|
} => {
|
||||||
let idx = self.ensure_open(&room);
|
// Only record messages for rooms we currently have open; a
|
||||||
self.open[idx].lines.push(RoomLine::chat(username, message));
|
// message for a room we just left must not resurrect its tab.
|
||||||
|
if let Some(idx) = self.open_index(&room) {
|
||||||
|
self.open[idx]
|
||||||
|
.lines
|
||||||
|
.push(RoomLine::chat(username, message));
|
||||||
if viewing != Some(room.as_str()) {
|
if viewing != Some(room.as_str()) {
|
||||||
self.open[idx].unread += 1;
|
self.open[idx].unread += 1;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
}
|
||||||
RoomEvent::UserJoined { room, username } => {
|
RoomEvent::UserJoined { room, username } => {
|
||||||
if let Some(idx) = self.open_index(&room) {
|
if let Some(idx) = self.open_index(&room) {
|
||||||
if !self.open[idx].users.contains(&username) {
|
if !self.open[idx].users.contains(&username) {
|
||||||
@@ -377,6 +394,81 @@ mod tests {
|
|||||||
assert_eq!(state.active, 1);
|
assert_eq!(state.active, 1);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn message_for_unopened_room_is_dropped() {
|
||||||
|
let mut state = RoomsState::new();
|
||||||
|
state.apply_event(
|
||||||
|
RoomEvent::Message {
|
||||||
|
room: "jazz".to_string(),
|
||||||
|
username: "bob".to_string(),
|
||||||
|
message: "hi".to_string(),
|
||||||
|
},
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
assert!(
|
||||||
|
state.open.is_empty(),
|
||||||
|
"a message must not fabricate a tab for an unjoined room"
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn message_does_not_resurrect_a_closed_room() {
|
||||||
|
let mut state = RoomsState::new();
|
||||||
|
state.focus_or_open("jazz");
|
||||||
|
assert_eq!(state.close_active().as_deref(), Some("jazz"));
|
||||||
|
// A straggler message arrives after we left.
|
||||||
|
state.apply_event(
|
||||||
|
RoomEvent::Message {
|
||||||
|
room: "jazz".to_string(),
|
||||||
|
username: "bob".to_string(),
|
||||||
|
message: "bye".to_string(),
|
||||||
|
},
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
assert!(state.open.is_empty(), "left room must stay closed");
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn list_refresh_keeps_highlight_on_the_same_room() {
|
||||||
|
let mut state = RoomsState::new();
|
||||||
|
state.apply_event(
|
||||||
|
RoomEvent::List(vec![
|
||||||
|
room("jazz", 5),
|
||||||
|
room("blues", 3),
|
||||||
|
room("ambient", 1),
|
||||||
|
]),
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
// Highlight "ambient" (index 2 in busiest-first order).
|
||||||
|
state.list_selected = 2;
|
||||||
|
assert_eq!(state.selected_room_name().as_deref(), Some("ambient"));
|
||||||
|
// A refresh reorders ambient to the top; the highlight should follow.
|
||||||
|
state.apply_event(
|
||||||
|
RoomEvent::List(vec![
|
||||||
|
room("jazz", 5),
|
||||||
|
room("blues", 3),
|
||||||
|
room("ambient", 50),
|
||||||
|
]),
|
||||||
|
None,
|
||||||
|
);
|
||||||
|
assert_eq!(state.selected_room_name().as_deref(), Some("ambient"));
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn closing_a_tab_clears_unread_on_the_newly_active_room() {
|
||||||
|
let mut state = RoomsState::new();
|
||||||
|
state.focus_or_open("a");
|
||||||
|
state.focus_or_open("b"); // active = 1
|
||||||
|
state.open[0].unread = 3; // unread accrued on "a" while viewing "b"
|
||||||
|
assert_eq!(state.close_active().as_deref(), Some("b"));
|
||||||
|
assert_eq!(state.active, 0);
|
||||||
|
assert_eq!(
|
||||||
|
state.open[0].unread, 0,
|
||||||
|
"landing on room a should clear its badge"
|
||||||
|
);
|
||||||
|
assert_eq!(state.total_unread(), 0);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn user_joined_and_left_update_membership() {
|
fn user_joined_and_left_update_membership() {
|
||||||
let mut state = RoomsState::new();
|
let mut state = RoomsState::new();
|
||||||
|
|||||||
@@ -777,6 +777,12 @@ impl MainTui {
|
|||||||
fn handle_rooms_list_input(&mut self, key: KeyEvent) {
|
fn handle_rooms_list_input(&mut self, key: KeyEvent) {
|
||||||
let len = self.state.rooms.filtered_rooms().len();
|
let len = self.state.rooms.filtered_rooms().len();
|
||||||
match key.code {
|
match key.code {
|
||||||
|
// Esc peels back one level: clear a lingering filter first (as the
|
||||||
|
// title's "Esc: clear" promises), otherwise close the popup.
|
||||||
|
KeyCode::Esc if !self.state.rooms.list_filter.is_empty() => {
|
||||||
|
self.state.rooms.list_filter.clear();
|
||||||
|
self.state.rooms.list_selected = 0;
|
||||||
|
}
|
||||||
KeyCode::Esc | KeyCode::Char('q') => {
|
KeyCode::Esc | KeyCode::Char('q') => {
|
||||||
self.state.show_rooms = false;
|
self.state.show_rooms = false;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -10,7 +10,7 @@ use ratatui::{
|
|||||||
text::{Line, Span},
|
text::{Line, Span},
|
||||||
widgets::{
|
widgets::{
|
||||||
Block, Borders, Cell, HighlightSpacing, Paragraph, Row, Table,
|
Block, Borders, Cell, HighlightSpacing, Paragraph, Row, Table,
|
||||||
TableState, Wrap,
|
TableState,
|
||||||
},
|
},
|
||||||
};
|
};
|
||||||
|
|
||||||
@@ -181,7 +181,10 @@ fn render_messages(
|
|||||||
area: Rect,
|
area: Rect,
|
||||||
lines: &[crate::models::RoomLine],
|
lines: &[crate::models::RoomLine],
|
||||||
) {
|
) {
|
||||||
// Auto-scroll: keep the most recent lines that fit the height.
|
// Auto-scroll: show the most recent lines. Each RoomLine renders as exactly
|
||||||
|
// one row (no wrapping) so the tail count is exact and the newest message
|
||||||
|
// is always visible; over-long lines are clipped at the right edge rather
|
||||||
|
// than wrapping and pushing newer lines off the bottom.
|
||||||
let height = area.height as usize;
|
let height = area.height as usize;
|
||||||
let start = lines.len().saturating_sub(height.max(1));
|
let start = lines.len().saturating_sub(height.max(1));
|
||||||
let rendered: Vec<Line> = lines[start..]
|
let rendered: Vec<Line> = lines[start..]
|
||||||
@@ -194,10 +197,7 @@ fn render_messages(
|
|||||||
None => Line::from(Span::styled(l.text.clone(), dimmed_style())),
|
None => Line::from(Span::styled(l.text.clone(), dimmed_style())),
|
||||||
})
|
})
|
||||||
.collect();
|
.collect();
|
||||||
frame.render_widget(
|
frame.render_widget(Paragraph::new(rendered), area);
|
||||||
Paragraph::new(rendered).wrap(Wrap { trim: false }),
|
|
||||||
area,
|
|
||||||
);
|
|
||||||
}
|
}
|
||||||
|
|
||||||
fn render_users(frame: &mut Frame, area: Rect, users: &[String]) {
|
fn render_users(frame: &mut Frame, area: Rect, users: &[String]) {
|
||||||
|
|||||||
Reference in New Issue
Block a user