From 38a69b559733b0bc2235a36d9936529fc977af22 Mon Sep 17 00:00:00 2001 From: Mikael Nordfeldth Date: Wed, 16 Oct 2013 13:55:32 +0200 Subject: [PATCH] Better checks during User::register and improved Nickname checks --- classes/User.php | 53 +++++--------------- lib/nickname.php | 125 ++++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 123 insertions(+), 55 deletions(-) diff --git a/classes/User.php b/classes/User.php index 1508ed97b2..33fabf8296 100644 --- a/classes/User.php +++ b/classes/User.php @@ -177,38 +177,6 @@ class User extends Managed_DataObject return $result; } - /** - * Check whether the given nickname is potentially usable, or if it's - * excluded by any blacklists on this system. - * - * WARNING: INPUT IS NOT VALIDATED OR NORMALIZED. NON-NORMALIZED INPUT - * OR INVALID INPUT MAY LEAD TO FALSE RESULTS. - * - * @param string $nickname - * @return boolean true if clear, false if blacklisted - */ - static function allowed_nickname($nickname) - { - // XXX: should already be validated for size, content, etc. - $blacklist = common_config('nickname', 'blacklist'); - - //all directory and file names should be blacklisted - $d = dir(INSTALLDIR); - while (false !== ($entry = $d->read())) { - $blacklist[]=$entry; - } - $d->close(); - - //all top level names in the router should be blacklisted - $router = Router::get(); - foreach(array_keys($router->m->getPaths()) as $path){ - if(preg_match('/^\/(.*?)[\/\?]/',$path,$matches)){ - $blacklist[]=$matches[1]; - } - } - return !in_array($nickname, $blacklist); - } - /** * Get the most recent notice posted by this user, if any. * @@ -258,19 +226,18 @@ class User extends Managed_DataObject $profile = new Profile(); - if(!empty($email)) - { + if (!empty($email)) { $email = common_canonical_email($email); } - $nickname = common_canonical_nickname($nickname); - $profile->nickname = $nickname; - if(! User::allowed_nickname($nickname)){ - common_log(LOG_WARNING, sprintf("Attempted to register a nickname that is not allowed: %s", $profile->nickname), - __FILE__); + try { + $profile->nickname = Nickname::normalize($nickname, true); + } catch (NicknameException $e) { + common_log(LOG_WARNING, sprintf('Bad nickname during User registration for %s: %s', $profile->nickname, $e->getMessage()), __FILE__); return false; } - $profile->profileurl = common_profile_url($nickname); + + $profile->profileurl = common_profile_url($profile->nickname); if (!empty($fullname)) { $profile->fullname = $fullname; @@ -298,7 +265,7 @@ class User extends Managed_DataObject $user = new User(); - $user->nickname = $nickname; + $user->nickname = $profile->nickname; $invite = null; @@ -338,7 +305,6 @@ class User extends Managed_DataObject $profile->query('BEGIN'); $id = $profile->insert(); - if (empty($id)) { common_log_db_error($profile, 'INSERT', __FILE__); return false; @@ -360,6 +326,7 @@ class User extends Managed_DataObject if (!$result) { common_log_db_error($user, 'INSERT', __FILE__); + $profile->query('ROLLBACK'); return false; } @@ -388,6 +355,7 @@ class User extends Managed_DataObject if (!$result) { common_log_db_error($subscription, 'INSERT', __FILE__); + $profile->query('ROLLBACK'); return false; } @@ -409,6 +377,7 @@ class User extends Managed_DataObject if (!$result) { common_log_db_error($confirm, 'INSERT', __FILE__); + $profile->query('ROLLBACK'); return false; } } diff --git a/lib/nickname.php b/lib/nickname.php index e05e58c83b..2b8249d59a 100644 --- a/lib/nickname.php +++ b/lib/nickname.php @@ -76,32 +76,39 @@ class Nickname * * Note that valid nicknames may be in use or reserved. * - * @param string $str - * @return boolean + * @param string $str The nickname string to test + * @param boolean $checkuse Check if it's in use (return false if it is) + * + * @return boolean True if nickname is valid. False if invalid (or taken if checkuse==true). */ - public static function isValid($str) + public static function isValid($str, $checkuse=false) { try { - self::normalize($str); - return true; + self::normalize($str, $checkuse); } catch (NicknameException $e) { return false; } + + return true; } /** * Validate an input nickname string, and normalize it to its canonical form. * The canonical form will be returned, or an exception thrown if invalid. * - * @param string $str + * @param string $str The nickname string to test + * @param boolean $checkuse Check if it's in use (return false if it is) * @return string Normalized canonical form of $str * * @throws NicknameException (base class) - * @throws NicknameInvalidException + * @throws NicknameBlacklistedException * @throws NicknameEmptyException + * @throws NicknameInvalidException + * @throws NicknamePathCollisionException + * @throws NicknameTakenException * @throws NicknameTooLongException */ - public static function normalize($str) + public static function normalize($str, $checkuse=false) { if (mb_strlen($str) > self::MAX_LEN) { // Display forms must also fit! @@ -118,8 +125,16 @@ class Nickname if (!self::isCanonical($str)) { throw new NicknameInvalidException(); } - if (!User::allowed_nickname($str)) { - throw new NicknameBlacklistException(); + if (self::isBlacklisted($str)) { + throw new NicknameBlacklistedException(); + } + if (self::isSystemPath($str)) { + throw new NicknamePathCollisionException(); + } + if ($checkuse && $user = self::isTaken($str)) { + if ($user instanceof User) { + throw new NicknameTakenException(); + } } return $str; @@ -135,6 +150,59 @@ class Nickname { return preg_match('/^(?:' . self::CANONICAL_FMT . ')$/', $str); } + + /** + * Is the given string in our nickname blacklist? + * + * @param string $str + * @return boolean + */ + public static function isBlacklisted($str) + { + $blacklist = common_config('nickname', 'blacklist'); + return in_array($str, $blacklist); + } + + /** + * Is the given string identical to a system path or route? + * This could probably be put in some other class, but at + * at the moment, only Nickname requires this functionality. + * + * @param string $str + * @return boolean + */ + public static function isSystemPath($str) + { + $paths = array(); + + // All directory and file names in site root should be blacklisted + $d = dir(INSTALLDIR); + while (false !== ($entry = $d->read())) { + $paths[] = $entry; + } + $d->close(); + + // All top level names in the router should be blacklisted + $router = Router::get(); + foreach (array_keys($router->m->getPaths()) as $path) { + if (preg_match('/^\/(.*?)[\/\?]/',$path,$matches)) { + $paths[] = $matches[1]; + } + } + return in_array($str, $paths); + } + + /** + * Is the nickname already in use locally? Checks the User table. + * + * @param string $str + * @return User|null Returns null if no such user, otherwise a User object + */ + public static function isTaken($str) + { + $user = User::getKV('nickname', $str); + return $user; // null if no such User entry + } } class NicknameException extends ClientException @@ -169,7 +237,7 @@ class NicknameInvalidException extends NicknameException { } } -class NicknameEmptyException extends NicknameException +class NicknameEmptyException extends NicknameInvalidException { /** * Default localized message for this type of exception. @@ -198,11 +266,42 @@ class NicknameTooLongException extends NicknameInvalidException } } -class NicknameBlacklistException extends NicknameInvalidException +class NicknameBlacklistedException extends NicknameException { protected function defaultMessage() { // TRANS: Validation error in form for registration, profile and group settings, etc. - return _('Nickname is disallowed through blacklist or system path list.'); + return _('Nickname is disallowed through blacklist.'); + } +} + +class NicknamePathCollisionException extends NicknameException +{ + protected function defaultMessage() + { + // TRANS: Validation error in form for registration, profile and group settings, etc. + return _('Nickname is identical to system path names.'); + } +} + +class NicknameTakenException extends NicknameException +{ + public $user = null; // the User which occupies the nickname + + public function __construct(User $user, $msg=null, $code=400) + { + $this->byuser = $user; + + if ($msg === null) { + $msg = $this->defaultMessage(); + } + + parent::__construct($msg, $code); + } + + protected function defaultMessage() + { + // TRANS: Validation error in form for registration, profile and group settings, etc. + return _('Nickname is already in use on this server.'); } }