Prevent setting an empty username with 'USER [ * * :foo'

This used to be valid because '[' is stripped from usernames

This uses the ERR_INVALIDUSERNAME numeric I am about to introduce to
modern.ircdocs.horse: ircdocs/modern-irc#217

Author: progval

sable_ircd/src/command/client_command.rs

@@ -225,7 +225,8 @@ impl ClientCommand {
                 LookupError::NoSuchChannelName(name) => Some(make_numeric!(NoSuchChannel, &name)),
                 _ => None,
             },
-            CommandError::InvalidNick(name) => Some(make_numeric!(ErroneousNickname, &name)),
+            CommandError::InvalidNickname(name) => Some(make_numeric!(ErroneousNickname, &name)),
+            CommandError::InvalidUsername(_name) => Some(make_numeric!(InvalidUsername)),
             CommandError::InvalidChannelName(name) => {
                 Some(make_numeric!(InvalidChannelName, &name))
             }

sable_ircd/src/command/error.rs

@@ -27,7 +27,9 @@ pub enum CommandError {
     /// A required object wasn't found in the network state
     LookupError(LookupError),
     /// A nickname parameter wasn't a valid nick
-    InvalidNick(String),
+    InvalidNickname(String),
+    /// A username parameter wasn't a valid username
+    InvalidUsername(String),
     /// A channel name parameter wasn't a valid channel name
     InvalidChannelName(String),
     /// A services command was executed, but services aren't currently running
@@ -120,7 +122,13 @@ impl From<LookupError> for CommandError {
 
 impl From<InvalidNicknameError> for CommandError {
     fn from(e: InvalidNicknameError) -> Self {
-        Self::InvalidNick(e.0)
+        Self::InvalidNickname(e.0)
+    }
+}
+
+impl From<InvalidUsernameError> for CommandError {
+    fn from(e: InvalidUsernameError) -> Self {
+        Self::InvalidUsername(e.0)
     }
 }
 

sable_ircd/src/command/handlers/services/mod.rs

@@ -105,9 +105,12 @@ impl<'a> Command for ServicesCommand<'a> {
                     }
                 }
             }
-            CommandError::InvalidNick(name) => {
+            CommandError::InvalidNickname(name) => {
                 self.notice(format_args!("Invalid nickname {}", name));
             }
+            CommandError::InvalidUsername(name) => {
+                self.notice(format_args!("Invalid username {}", name));
+            }
             CommandError::InvalidChannelName(name) => {
                 self.notice(format_args!("Invalid channel name {}", name));
             }

sable_ircd/src/command/handlers/user.rs

@@ -5,14 +5,14 @@ fn handle_user(
     server: &ClientServer,
     source: PreClientSource,
     cmd: &dyn Command,
-    username: &str,
+    username: Username,
     _unused1: &str,
     _unused2: &str,
     realname: &str,
 ) -> CommandResult {
     // Ignore these results; they'll only fail if USER was already successfully processed
     // from this pre-client. If that happens we silently ignore the new values.
-    source.user.set(Username::new_coerce(username)).ok();
+    source.user.set(username).ok();
     source.realname.set(realname.to_owned()).ok();
 
     if source.can_register() {

sable_ircd/src/command/plumbing/argument_type.rs

@@ -52,6 +52,12 @@ impl<'a> PositionalArgument<'a> for Nickname {
     }
 }
 
+impl<'a> PositionalArgument<'a> for Username {
+    fn parse_str(_ctx: &'a dyn Command, value: &'a str) -> Result<Self, CommandError> {
+        Ok(Username::new_coerce(value)?)
+    }
+}
+
 impl<'a> PositionalArgument<'a> for state::ChannelRoleName {
     fn parse_str(_ctx: &'a dyn Command, value: &'a str) -> Result<Self, CommandError> {
         value

sable_ircd/src/messages/numeric.rs

@@ -64,6 +64,7 @@ define_messages! {
     451(NotRegistered)          => { ()                         => ":You have not registered" },
     461(NotEnoughParameters)    => { (command: &str)            => "{command} :Not enough parameters" },
     462(AlreadyRegistered)      => { ()                         => ":You are already connected and cannot handshake again" },
+    468(InvalidUsername)        => { ()                         => ":Your username is not valid" },
     472(UnknownMode)            => { (c: char)                  => "{c} :Unknown mode character" },
     479(InvalidChannelName)     => { (name: &str)               => "{name} :Illegal channel name" },
     482(ChanOpPrivsNeeded)      => { (chan: &ChannelName)       => "{chan} :You're not a channel operator" },

sable_network/src/validated.rs

@@ -59,7 +59,11 @@ define_validated! {
     }
 
     Username(ArrayString<{ Username::LENGTH }>) {
-        Ok(())
+        if value.len() == 0 {
+            Self::error(value)
+        } else {
+            Ok(())
+        }
     }
 
     Hostname(ArrayString<{ Hostname::LENGTH }>) {
@@ -129,13 +133,14 @@ impl Username {
     pub const LENGTH: usize = 10;
 
     /// Coerce the provided value into a valid `Username`, by truncating to the
-    /// permitted length and removing any invalid characters.
-    pub fn new_coerce(s: &str) -> Self {
+    /// permitted length, removing any invalid characters, and checking it is not empty.
+    pub fn new_coerce(s: &str) -> <Self as Validated>::Result {
         let mut s = s.to_string();
         s.retain(|c| c != '[');
         s.truncate(s.floor_char_boundary(Self::LENGTH));
         // expect() is safe here; we've already truncated to the max length
-        Self(ArrayString::try_from(s.as_str()).expect("Failed to convert string"))
+        let val = ArrayString::try_from(s.as_str()).expect("Failed to convert string");
+        Self::validate(&val).map(|()| Self(val))
     }
 }