darkirc: Reject malformed TOPIC and NickServ commands

bin/darkirc/src/irc/command.rs

@@ -63,6 +63,38 @@ use super::{
 };
 use crate::crypto::bcrypt::bcrypt_hash_password;
 
+#[derive(Debug, PartialEq, Eq)]
+enum TopicRequest<'a> {
+    Get { channel: &'a str },
+    Set { channel: &'a str, topic: &'a str },
+}
+
+/// Parse a TOPIC command without accepting ambiguous topic bodies.
+fn parse_topic_request(args: &str) -> Option<TopicRequest<'_>> {
+    let args = args.trim_start_matches(|c: char| c.is_ascii_whitespace());
+    if args.is_empty() {
+        return None
+    }
+
+    let (channel, rest) = match args.find(|c: char| c.is_ascii_whitespace()) {
+        Some(idx) => {
+            (&args[..idx], args[idx..].trim_start_matches(|c: char| c.is_ascii_whitespace()))
+        }
+        None => (args, ""),
+    };
+
+    if channel.is_empty() {
+        return None
+    }
+
+    if rest.is_empty() {
+        return Some(TopicRequest::Get { channel })
+    }
+
+    let topic = rest.strip_prefix(':')?;
+    Some(TopicRequest::Set { channel, topic })
+}
+
 impl Client {
     /// `ADMIN [<server>]`
     ///
@@ -695,9 +727,7 @@ impl Client {
         }
 
         let nick = self.nickname.read().await.to_string();
-        let mut tokens = args.split_ascii_whitespace();
-
-        let Some(channel) = tokens.next() else {
+        let Some(request) = parse_topic_request(args) else {
             self.penalty.fetch_add(1, SeqCst);
             return Ok(vec![ReplyType::Server((
                 ERR_NEEDMOREPARAMS,
@@ -705,37 +735,42 @@ impl Client {
             ))])
         };
 
-        if !self.server.channels.read().await.contains_key(channel) {
-            return Ok(vec![ReplyType::Server((
-                ERR_NOSUCHCHANNEL,
-                format!("{nick} {channel} :No such channel"),
-            ))])
-        }
+        match request {
+            TopicRequest::Get { channel } => {
+                let channels = self.server.channels.read().await;
+                let Some(channel_state) = channels.get(channel) else {
+                    return Ok(vec![ReplyType::Server((
+                        ERR_NOSUCHCHANNEL,
+                        format!("{nick} {channel} :No such channel"),
+                    ))])
+                };
 
-        // If there's a topic, we'll set it, otherwise return the set topic.
-        let Some(topic) = tokens.next() else {
-            let topic = self.server.channels.read().await.get(channel).unwrap().topic.clone();
-            if topic.is_empty() {
-                return Ok(vec![ReplyType::Server((
-                    RPL_NOTOPIC,
-                    format!("{nick} {channel} :No topic is set"),
-                ))])
-            } else {
-                return Ok(vec![ReplyType::Server((
-                    RPL_TOPIC,
-                    format!("{nick} {channel} :{topic}"),
-                ))])
+                if channel_state.topic.is_empty() {
+                    Ok(vec![ReplyType::Server((
+                        RPL_NOTOPIC,
+                        format!("{nick} {channel} :No topic is set"),
+                    ))])
+                } else {
+                    Ok(vec![ReplyType::Server((
+                        RPL_TOPIC,
+                        format!("{nick} {channel} :{}", channel_state.topic),
+                    ))])
+                }
             }
-        };
-
-        // Set the new topic
-        self.server.channels.write().await.get_mut(channel).unwrap().topic =
-            topic.strip_prefix(':').unwrap().to_string();
+            TopicRequest::Set { channel, topic } => {
+                let mut channels = self.server.channels.write().await;
+                let Some(channel_state) = channels.get_mut(channel) else {
+                    return Ok(vec![ReplyType::Server((
+                        ERR_NOSUCHCHANNEL,
+                        format!("{nick} {channel} :No such channel"),
+                    ))])
+                };
 
-        // Send reply
-        let replies = vec![ReplyType::Client((nick, format!("TOPIC {channel} {topic}")))];
+                channel_state.topic = topic.to_string();
 
-        Ok(replies)
+                Ok(vec![ReplyType::Client((nick, format!("TOPIC {channel} :{topic}")))])
+            }
+        }
     }
 
     /// `USER <user> <mode> <unused> <realname>`
@@ -982,3 +1017,34 @@ impl Client {
         Ok(replies)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{parse_topic_request, TopicRequest};
+
+    #[test]
+    fn parse_topic_request_gets_current_topic() {
+        assert_eq!(parse_topic_request("#chan"), Some(TopicRequest::Get { channel: "#chan" }));
+    }
+
+    #[test]
+    fn parse_topic_request_sets_colon_prefixed_topic() {
+        assert_eq!(
+            parse_topic_request("#chan :hello world"),
+            Some(TopicRequest::Set { channel: "#chan", topic: "hello world" })
+        );
+    }
+
+    #[test]
+    fn parse_topic_request_allows_empty_topic() {
+        assert_eq!(
+            parse_topic_request("#chan :"),
+            Some(TopicRequest::Set { channel: "#chan", topic: "" })
+        );
+    }
+
+    #[test]
+    fn parse_topic_request_rejects_bare_topic_body() {
+        assert_eq!(parse_topic_request("#chan value"), None);
+    }
+}

bin/darkirc/src/irc/services/nickserv.rs

@@ -255,16 +255,12 @@ impl NickServ {
     /// Called from `command::handle_cmd_privmsg`.
     pub async fn handle_query(&self, query: &str) -> Result<Vec<ReplyType>> {
         let nick = self.nickname.read().await.to_string();
-        let mut tokens = query.split_ascii_whitespace();
-
-        tokens.next();
-        let Some(command) = tokens.next() else {
+        let Some((command, mut tokens)) = parse_nickserv_command(query) else {
             return Ok(vec![ReplyType::Server((
                 ERR_NOTEXTTOSEND,
                 format!("{nick} :No text to send"),
             ))])
         };
-        let command = command.strip_prefix(':').unwrap();
 
         match command.to_uppercase().as_str() {
             "INFO" => self.handle_info(&nick, &mut tokens).await,
@@ -416,16 +412,13 @@ impl NickServ {
         tokens: &mut SplitAsciiWhitespace<'_>,
     ) -> Result<Vec<ReplyType>> {
         // Gather the tokens
-        let account_name = tokens.next();
-        let identity_nullifier = tokens.next();
-        let identity_trapdoor = tokens.next();
-        let user_msg_limit = tokens.next();
-
-        if account_name.is_none() ||
-            identity_nullifier.is_none() ||
-            identity_trapdoor.is_none() ||
-            user_msg_limit.is_none()
-        {
+        let (
+            Some(account_name),
+            Some(identity_nullifier),
+            Some(identity_trapdoor),
+            Some(user_msg_limit),
+        ) = (tokens.next(), tokens.next(), tokens.next(), tokens.next())
+        else {
             return Ok(notices(
                 nick,
                 [
@@ -437,10 +430,6 @@ impl NickServ {
             ))
         };
 
-        let account_name = account_name.unwrap();
-        let identity_nullifier = identity_nullifier.unwrap();
-        let identity_trapdoor = identity_trapdoor.unwrap();
-
         // Reserved name. We use `default` for the mirror tree.
         if account_name == "default" || account_name.is_empty() {
             return Ok(vec![notice(nick, "Invalid account name.")])
@@ -449,7 +438,7 @@ impl NickServ {
         // Parse user_msg_limit defensively. The original code
         // panicked here, which would tear down the whole IRC
         // session on a typo.
-        let user_msg_limit: u64 = match user_msg_limit.unwrap().parse() {
+        let user_msg_limit: u64 = match user_msg_limit.parse() {
             Ok(v) => v,
             Err(_) => {
                 return Ok(vec![notice(nick, "Invalid user_msg_limit: must be a positive integer.")])
@@ -885,6 +874,19 @@ impl NickServ {
     }
 }
 
+/// Parse a NickServ PRIVMSG body into the service command and remaining arguments.
+fn parse_nickserv_command(query: &str) -> Option<(&str, SplitAsciiWhitespace<'_>)> {
+    let mut tokens = query.split_ascii_whitespace();
+    tokens.next()?;
+
+    let command = tokens.next()?.strip_prefix(':')?;
+    if command.is_empty() {
+        return None
+    }
+
+    Some((command, tokens))
+}
+
 /// Decode a base58-encoded `pallas::Base` scalar. Returns `None`
 /// for any malformed input rather than panicking - this is called
 /// on user-supplied IRC tokens.
@@ -893,3 +895,28 @@ fn parse_pallas_b58(s: &str) -> Option<pallas::Base> {
     let arr: [u8; 32] = bytes.try_into().ok()?;
     pallas::Base::from_repr(arr).into_option()
 }
+
+#[cfg(test)]
+mod tests {
+    use super::parse_nickserv_command;
+
+    #[test]
+    fn parse_nickserv_command_accepts_colon_prefixed_command() {
+        let (command, mut tokens) =
+            parse_nickserv_command("NickServ :REGISTER alice n t 100").unwrap();
+
+        assert_eq!(command, "REGISTER");
+        assert_eq!(tokens.next(), Some("alice"));
+        assert_eq!(tokens.next(), Some("n"));
+    }
+
+    #[test]
+    fn parse_nickserv_command_rejects_bare_command() {
+        assert!(parse_nickserv_command("NickServ REGISTER alice").is_none());
+    }
+
+    #[test]
+    fn parse_nickserv_command_rejects_empty_command() {
+        assert!(parse_nickserv_command("NickServ :").is_none());
+    }
+}