From 96cbf60c3f5dc4cb4f2d049874234edc88a1fb5b Mon Sep 17 00:00:00 2001 From: Joshua Barretto Date: Thu, 17 Jun 2021 19:55:21 +0100 Subject: [PATCH] Made commands a distinct ClientMsg to avoid possible sanitisation problems for clients --- Cargo.lock | 7 +++++++ client/src/lib.rs | 11 ++++++++--- common/net/src/msg/client.rs | 5 ++++- common/src/event.rs | 2 +- server/src/cmd.rs | 9 +++++++-- server/src/events/mod.rs | 8 ++++---- server/src/lib.rs | 23 ++++++++--------------- server/src/sys/msg/general.rs | 12 ++++++------ voxygen/Cargo.toml | 1 + voxygen/src/hud/chat.rs | 13 ++++++++++++- voxygen/src/hud/mod.rs | 4 ++++ voxygen/src/session/mod.rs | 3 +++ 12 files changed, 65 insertions(+), 33 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 37398ea1ff..4a55a6ba37 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -707,6 +707,12 @@ dependencies = [ "memchr", ] +[[package]] +name = "comma" +version = "0.1.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "96677551532ffe910f470bd767a9a7daf9ba53b1f5532e0891dba6c735f692e5" + [[package]] name = "concurrent-queue" version = "1.2.2" @@ -6049,6 +6055,7 @@ dependencies = [ "bincode", "bytemuck", "chrono", + "comma", "conrod_core", "conrod_winit", "copy_dir", diff --git a/client/src/lib.rs b/client/src/lib.rs index 9f09ffb9d0..de2f3d56b4 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -808,9 +808,9 @@ impl Client { //Only in game, terrain ClientGeneral::TerrainChunkRequest { .. } => &mut self.terrain_stream, //Always possible - ClientGeneral::ChatMsg(_) | ClientGeneral::Terminate => { - &mut self.general_stream - }, + ClientGeneral::ChatMsg(_) + | ClientGeneral::Command(_, _) + | ClientGeneral::Terminate => &mut self.general_stream, }; stream.send(msg) }, @@ -1370,6 +1370,11 @@ impl Client { } } + /// Send a command to the server. + pub fn send_command(&mut self, name: String, args: Vec) { + self.send_msg(ClientGeneral::Command(name, args)); + } + /// Remove all cached terrain pub fn clear_terrain(&mut self) { self.state.clear_terrain(); diff --git a/common/net/src/msg/client.rs b/common/net/src/msg/client.rs index 9252e5ec85..0cc811ceb9 100644 --- a/common/net/src/msg/client.rs +++ b/common/net/src/msg/client.rs @@ -80,6 +80,7 @@ pub enum ClientGeneral { }, //Always possible ChatMsg(String), + Command(String, Vec), Terminate, RequestPlayerPhysics { server_authoritative: bool, @@ -129,7 +130,9 @@ impl ClientMsg { c_type == ClientType::Game && presence.is_some() }, //Always possible - ClientGeneral::ChatMsg(_) | ClientGeneral::Terminate => true, + ClientGeneral::ChatMsg(_) + | ClientGeneral::Command(_, _) + | ClientGeneral::Terminate => true, } }, ClientMsg::Ping(_) => true, diff --git a/common/src/event.rs b/common/src/event.rs index ffa178fdd4..efb1ddf535 100644 --- a/common/src/event.rs +++ b/common/src/event.rs @@ -140,7 +140,7 @@ pub enum ServerEvent { ClientDisconnect(EcsEntity, DisconnectReason), ClientDisconnectWithoutPersistence(EcsEntity), ChunkRequest(EcsEntity, Vec2), - ChatCmd(EcsEntity, String), + ChatCmd(EcsEntity, String, Vec), /// Send a chat message to the player from an npc or other player Chat(comp::UnresolvedChatMsg), Aura { diff --git a/server/src/cmd.rs b/server/src/cmd.rs index fcdd197e1a..a10e1276d1 100644 --- a/server/src/cmd.rs +++ b/server/src/cmd.rs @@ -52,11 +52,15 @@ use scan_fmt::{scan_fmt, scan_fmt_some}; use tracing::{error, info, warn}; pub trait ChatCommandExt { - fn execute(&self, server: &mut Server, entity: EcsEntity, args: String); + fn execute(&self, server: &mut Server, entity: EcsEntity, args: Vec); } impl ChatCommandExt for ChatCommand { #[allow(clippy::needless_return)] // TODO: Pending review in #587 - fn execute(&self, server: &mut Server, entity: EcsEntity, args: String) { + fn execute(&self, server: &mut Server, entity: EcsEntity, args: Vec) { + // TODO: Pass arguments to commands as Vec, not String, to support + // proper parsing. + let args = args.join(" "); + if let Err(err) = do_command(server, entity, entity, args, self) { server.notify_client( entity, @@ -101,6 +105,7 @@ fn do_command( cmd.keyword() )); } + let handler: CommandHandler = match cmd { ChatCommand::Adminify => handle_adminify, ChatCommand::Airship => handle_spawn_airship, diff --git a/server/src/events/mod.rs b/server/src/events/mod.rs index 7ab52e6b60..387f8c0fa1 100644 --- a/server/src/events/mod.rs +++ b/server/src/events/mod.rs @@ -188,8 +188,8 @@ impl Server { ServerEvent::ChunkRequest(entity, key) => { requested_chunks.push((entity, key)); }, - ServerEvent::ChatCmd(entity, cmd) => { - chat_commands.push((entity, cmd)); + ServerEvent::ChatCmd(entity, name, args) => { + chat_commands.push((entity, name, args)); }, ServerEvent::Chat(msg) => { chat_messages.push(msg); @@ -229,8 +229,8 @@ impl Server { self.generate_chunk(entity, key); } - for (entity, cmd) in chat_commands { - self.process_chat_cmd(entity, cmd); + for (entity, name, args) in chat_commands { + self.process_chat_cmd(entity, name, args); } for msg in chat_messages { diff --git a/server/src/lib.rs b/server/src/lib.rs index b7ca95298c..5f5a944863 100644 --- a/server/src/lib.rs +++ b/server/src/lib.rs @@ -983,16 +983,9 @@ impl Server { ); } - fn process_chat_cmd(&mut self, entity: EcsEntity, cmd: String) { - // Separate string into keyword and arguments. - let sep = cmd.find(' '); - let (kwd, args) = match sep { - Some(i) => (cmd[..i].to_string(), cmd[(i + 1)..].to_string()), - None => (cmd, "".to_string()), - }; - + fn process_chat_cmd(&mut self, entity: EcsEntity, name: String, args: Vec) { // Find the command object and run its handler. - if let Ok(command) = kwd.parse::() { + if let Ok(command) = name.parse::() { command.execute(self, entity, args); } else { #[cfg(feature = "plugins")] @@ -1020,8 +1013,8 @@ impl Server { let rs = plugin_manager.execute_event( &ecs_world, &plugin_api::event::ChatCommandEvent { - command: kwd.clone(), - command_args: args.split(' ').map(|x| x.to_owned()).collect(), + command: name.clone(), + command_args: args.clone(), player: plugin_api::event::Player { id: uid }, }, ); @@ -1035,7 +1028,7 @@ impl Server { format!( "Unknown command '/{}'.\nType '/help' for available \ commands", - kwd + name ), ), ); @@ -1059,7 +1052,7 @@ impl Server { comp::ChatType::CommandError, format!( "Error occurred while executing command '/{}'.\n{}", - kwd, e + name, e ), ), ); @@ -1068,7 +1061,7 @@ impl Server { } }, Err(e) => { - error!(?e, "Can't execute command {} {}", kwd, args); + error!(?e, "Can't execute command {} {:?}", name, args); self.notify_client( entity, ServerGeneral::server_msg( @@ -1076,7 +1069,7 @@ impl Server { format!( "Internal error while executing '/{}'.\nContact the server \ administrator", - kwd + name ), ), ); diff --git a/server/src/sys/msg/general.rs b/server/src/sys/msg/general.rs index 26eaebb489..664efef403 100644 --- a/server/src/sys/msg/general.rs +++ b/server/src/sys/msg/general.rs @@ -29,12 +29,7 @@ impl Sys { if player.is_some() { match validate_chat_msg(&message) { Ok(()) => { - if let Some(message) = message.strip_prefix('/') { - if !message.is_empty() { - let argv = String::from(message); - server_emitter.emit(ServerEvent::ChatCmd(entity, argv)); - } - } else if let Some(from) = uids.get(entity) { + if let Some(from) = uids.get(entity) { const CHAT_MODE_DEFAULT: &ChatMode = &ChatMode::default(); let mode = chat_modes.get(entity).unwrap_or(CHAT_MODE_DEFAULT); // Send chat message @@ -52,6 +47,11 @@ impl Sys { } } }, + ClientGeneral::Command(name, args) => { + if player.is_some() { + server_emitter.emit(ServerEvent::ChatCmd(entity, name, args)); + } + }, ClientGeneral::Terminate => { debug!(?entity, "Client send message to terminate session"); server_emitter.emit(ServerEvent::ClientDisconnect( diff --git a/voxygen/Cargo.toml b/voxygen/Cargo.toml index 150f3d35fb..70cb8f3b90 100644 --- a/voxygen/Cargo.toml +++ b/voxygen/Cargo.toml @@ -78,6 +78,7 @@ backtrace = "0.3.40" bincode = "1.3.1" chrono = { version = "0.4.9", features = ["serde"] } cpal = "0.13" +comma = "0.1" copy_dir = "0.1.2" crossbeam-utils = "0.8.1" crossbeam-channel = "0.5" diff --git a/voxygen/src/hud/chat.rs b/voxygen/src/hud/chat.rs index 9bcd04ce3a..43283d68a0 100644 --- a/voxygen/src/hud/chat.rs +++ b/voxygen/src/hud/chat.rs @@ -168,6 +168,7 @@ pub struct State { pub enum Event { TabCompletionStart(String), SendMessage(String), + SendCommand(String, Vec), Focus(Id), ChangeChatTab(Option), ShowChatTabSettings(usize), @@ -645,7 +646,17 @@ impl<'a> Widget for Chat<'a> { s.history.truncate(self.history_max); } }); - events.push(Event::SendMessage(msg)); + if let Some(msg) = msg.strip_prefix('/') { + match msg.parse::() { + Ok(cmd) => events.push(Event::SendCommand(cmd.name, cmd.arguments)), + Err(err) => self.new_messages.push_back(ChatMsg { + chat_type: ChatType::CommandError, + message: err.to_string(), + }), + } + } else { + events.push(Event::SendMessage(msg)); + } } events } diff --git a/voxygen/src/hud/mod.rs b/voxygen/src/hud/mod.rs index a012c63847..a61df245bc 100644 --- a/voxygen/src/hud/mod.rs +++ b/voxygen/src/hud/mod.rs @@ -375,6 +375,7 @@ pub struct HudInfo { #[derive(Clone)] pub enum Event { SendMessage(String), + SendCommand(String, Vec), CharacterSelection, UseSlot { @@ -2722,6 +2723,9 @@ impl Hud { chat::Event::SendMessage(message) => { events.push(Event::SendMessage(message)); }, + chat::Event::SendCommand(name, args) => { + events.push(Event::SendCommand(name, args)); + }, chat::Event::Focus(focus_id) => { self.to_focus = Some(Some(focus_id)); }, diff --git a/voxygen/src/session/mod.rs b/voxygen/src/session/mod.rs index 104459f2b8..d4e2e4cb39 100644 --- a/voxygen/src/session/mod.rs +++ b/voxygen/src/session/mod.rs @@ -1032,6 +1032,9 @@ impl PlayState for SessionState { // TODO: Handle result self.client.borrow_mut().send_chat(msg); }, + HudEvent::SendCommand(name, args) => { + self.client.borrow_mut().send_command(name, args); + }, HudEvent::CharacterSelection => { self.client.borrow_mut().request_remove_character() },