From 6c4e5a89c1dd15bd9b408dc3ef257fc16551ab0b Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 11:31:10 -0800 Subject: [PATCH 01/10] Add some doc comments on nickname-related stuff in util.php --- lib/util.php | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 57 insertions(+), 2 deletions(-) diff --git a/lib/util.php b/lib/util.php index ce5da1cd81..f5a4dc87b8 100644 --- a/lib/util.php +++ b/lib/util.php @@ -517,14 +517,32 @@ function common_user_cache_hash($user=false) } } -// get canonical version of nickname for comparison +/** + * get canonical version of nickname for comparison + * + * Currently this just runs strtolower(); more is needed. + * + * @fixme normalize punctuation chars where applicable + * @fixme reject invalid input + * + * @param string $nickname + * @return string + */ function common_canonical_nickname($nickname) { // XXX: UTF-8 canonicalization (like combining chars) return strtolower($nickname); } -// get canonical version of email for comparison +/** + * get canonical version of email for comparison + * + * @fixme actually normalize + * @fixme reject invalid input + * + * @param string $email + * @return string + */ function common_canonical_email($email) { // XXX: canonicalize UTF-8 @@ -532,6 +550,15 @@ function common_canonical_email($email) return $email; } +/** + * Partial notice markup rendering step: build links to !group references. + * + * @fixme use abstracted group nickname regex + * + * @param string $text partially rendered HTML + * @param Notice $notice in whose context we're working + * @return string partially rendered HTML + */ function common_render_content($text, $notice) { $r = common_render_text($text); @@ -597,6 +624,13 @@ function common_linkify_mention($mention) return $output; } +/** + * @fixme use NICKNAME_FMT more consistently + * + * @param string $text + * @param Notice $notice notice in whose context we're building links + * @return array + */ function common_find_mentions($text, $notice) { $mentions = array(); @@ -1026,6 +1060,27 @@ function common_group_link($sender_id, $nickname) } } +/** + * Resolve an ambiguous profile nickname reference, checking in following order: + * - profiles that $sender subscribes to + * - profiles that subscribe to $sender + * - local user profiles + * + * WARNING: does not validate or normalize $nickname -- MUST BE PRE-VALIDATED + * OR THERE MAY BE A RISK OF SQL INJECTION ATTACKS. THIS FUNCTION DOES NOT + * ESCAPE SQL. + * + * @fixme validate input + * @fixme escape SQL + * @fixme fix or remove mystery third parameter + * @fixme is $sender a User or Profile? + * + * @param $sender the user or profile in whose context we're looking + * @param string $nickname validated nickname of + * @param $dt unused mystery parameter. + * + * @return Profile or null + */ function common_relative_profile($sender, $nickname, $dt=null) { // Try to find profiles this profile is subscribed to that have this nickname From b7e0078d1078c045c2f609d948905e73eb184add Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 11:31:33 -0800 Subject: [PATCH 02/10] Start on some nickname-validation test cases: several of these fail right now because we had regressions in 0.8 or 0.9 where we lost normalization of uppercase and some other chars. --- tests/NicknameTest.php | 52 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 tests/NicknameTest.php diff --git a/tests/NicknameTest.php b/tests/NicknameTest.php new file mode 100644 index 0000000000..95af94098d --- /dev/null +++ b/tests/NicknameTest.php @@ -0,0 +1,52 @@ +assertEquals($expected, $norm, "normalized input nickname: $input -> $norm"); + } else { + $this->assertEquals($expected, false, "invalid input nickname: $input"); + } + } + + static public function provider() + { + return array( + array('evan', 'evan'), + array('Evan', 'evan'), + array('EVAN', 'evan'), + array('ev_an', 'evan'), + array('ev.an', 'evan'), + array('ev/an', false), + array('ev an', false), + array('ev-an', false), + array('évan', false), // so far... + array('Évan', false), // so far... + array('evan1', 'evan1'), + array('evan_1', 'evan1'), + ); + } +} From 6e249b4ab5aa292d78cfaa9be9dce8706e27ad80 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 11:57:27 -0800 Subject: [PATCH 03/10] doc comments on User::allowed_nickname --- classes/User.php | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/classes/User.php b/classes/User.php index 964bc3e7f3..92180a9fbc 100644 --- a/classes/User.php +++ b/classes/User.php @@ -116,6 +116,16 @@ class User extends Memcached_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. From dc350b5463e7d64a46d7f90143c2d001be99e280 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 14:15:25 -0800 Subject: [PATCH 04/10] Work in progress on nickname validation changes. lib/nickname.php appears to have been destroyed by NetBeans and will be rewritten shortly. Sigh. --- actions/apigroupcreate.php | 33 +-------- actions/editgroup.php | 15 +--- actions/newgroup.php | 19 ++---- actions/profilesettings.php | 16 ++--- actions/register.php | 11 ++- lib/common.php | 11 +++ lib/util.php | 44 ++++++++---- plugins/Facebook/FBConnectAuth.php | 15 ++-- plugins/Mapstraction/MapstractionPlugin.php | 4 +- plugins/Mapstraction/map.php | 2 +- plugins/OpenID/finishopenidlogin.php | 15 ++-- .../TwitterBridge/twitterauthorization.php | 14 ++-- tests/NicknameTest.php | 68 +++++++++++++++---- 13 files changed, 139 insertions(+), 128 deletions(-) diff --git a/actions/apigroupcreate.php b/actions/apigroupcreate.php index 54875a7188..d01504bc80 100644 --- a/actions/apigroupcreate.php +++ b/actions/apigroupcreate.php @@ -73,7 +73,7 @@ class ApiGroupCreateAction extends ApiAuthAction $this->user = $this->auth_user; - $this->nickname = $this->arg('nickname'); + $this->nickname = Nickname::normalize($this->arg('nickname')); $this->fullname = $this->arg('full_name'); $this->homepage = $this->arg('homepage'); $this->description = $this->arg('description'); @@ -150,26 +150,7 @@ class ApiGroupCreateAction extends ApiAuthAction */ function validateParams() { - $valid = Validate::string( - $this->nickname, array( - 'min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT - ) - ); - - if (!$valid) { - $this->clientError( - // TRANS: Validation error in form for group creation. - _( - 'Nickname must have only lowercase letters ' . - 'and numbers and no spaces.' - ), - 403, - $this->format - ); - return false; - } elseif ($this->groupNicknameExists($this->nickname)) { + if ($this->groupNicknameExists($this->nickname)) { $this->clientError( // TRANS: Client error trying to create a group with a nickname this is already in use. _('Nickname already in use. Try another one.'), @@ -265,15 +246,7 @@ class ApiGroupCreateAction extends ApiAuthAction foreach ($this->aliases as $alias) { - $valid = Validate::string( - $alias, array( - 'min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT - ) - ); - - if (!$valid) { + if (!Nickname::isValid($alias)) { $this->clientError( // TRANS: Client error shown when providing an invalid alias during group creation. // TRANS: %s is the invalid alias. diff --git a/actions/editgroup.php b/actions/editgroup.php index 4d3af34c7b..ab4dbb2836 100644 --- a/actions/editgroup.php +++ b/actions/editgroup.php @@ -177,21 +177,14 @@ class EditgroupAction extends GroupDesignAction return; } - $nickname = common_canonical_nickname($this->trimmed('nickname')); + $nickname = Nickname::normalize($this->trimmed('nickname')); $fullname = $this->trimmed('fullname'); $homepage = $this->trimmed('homepage'); $description = $this->trimmed('description'); $location = $this->trimmed('location'); $aliasstring = $this->trimmed('aliases'); - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - // TRANS: Group edit form validation error. - $this->showForm(_('Nickname must have only lowercase letters '. - 'and numbers and no spaces.')); - return; - } else if ($this->nicknameExists($nickname)) { + if ($this->nicknameExists($nickname)) { // TRANS: Group edit form validation error. $this->showForm(_('Nickname already in use. Try another one.')); return; @@ -241,9 +234,7 @@ class EditgroupAction extends GroupDesignAction } foreach ($aliases as $alias) { - if (!Validate::string($alias, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($alias)) { // TRANS: Group edit form validation error. $this->showForm(sprintf(_('Invalid alias: "%s"'), $alias)); return; diff --git a/actions/newgroup.php b/actions/newgroup.php index e0e7978c32..95af6415e5 100644 --- a/actions/newgroup.php +++ b/actions/newgroup.php @@ -113,21 +113,18 @@ class NewgroupAction extends Action function trySave() { - $nickname = $this->trimmed('nickname'); + try { + $nickname = Nickname::normalize($this->trimmed('nickname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); + } $fullname = $this->trimmed('fullname'); $homepage = $this->trimmed('homepage'); $description = $this->trimmed('description'); $location = $this->trimmed('location'); $aliasstring = $this->trimmed('aliases'); - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - // TRANS: Group create form validation error. - $this->showForm(_('Nickname must have only lowercase letters '. - 'and numbers and no spaces.')); - return; - } else if ($this->nicknameExists($nickname)) { + if ($this->nicknameExists($nickname)) { // TRANS: Group create form validation error. $this->showForm(_('Nickname already in use. Try another one.')); return; @@ -177,9 +174,7 @@ class NewgroupAction extends Action } foreach ($aliases as $alias) { - if (!Validate::string($alias, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($alias)) { // TRANS: Group create form validation error. $this->showForm(sprintf(_('Invalid alias: "%s"'), $alias)); return; diff --git a/actions/profilesettings.php b/actions/profilesettings.php index e1a0f8b6d0..28b1d20f34 100644 --- a/actions/profilesettings.php +++ b/actions/profilesettings.php @@ -225,7 +225,13 @@ class ProfilesettingsAction extends AccountSettingsAction if (Event::handle('StartProfileSaveForm', array($this))) { - $nickname = $this->trimmed('nickname'); + try { + $nickname = Nickname::normalize($this->trimmed('nickname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); + return; + } + $fullname = $this->trimmed('fullname'); $homepage = $this->trimmed('homepage'); $bio = $this->trimmed('bio'); @@ -236,13 +242,7 @@ class ProfilesettingsAction extends AccountSettingsAction $tagstring = $this->trimmed('tags'); // Some validation - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - // TRANS: Validation error in form for profile settings. - $this->showForm(_('Nickname must have only lowercase letters and numbers and no spaces.')); - return; - } else if (!User::allowed_nickname($nickname)) { + if (!User::allowed_nickname($nickname)) { // TRANS: Validation error in form for profile settings. $this->showForm(_('Not a valid nickname.')); return; diff --git a/actions/register.php b/actions/register.php index 3ae3f56f60..5d91aef70e 100644 --- a/actions/register.php +++ b/actions/register.php @@ -198,7 +198,11 @@ class RegisterAction extends Action } // Input scrubbing - $nickname = common_canonical_nickname($nickname); + try { + $nickname = Nickname::normalize($nickname); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); + } $email = common_canonical_email($email); if (!$this->boolean('license')) { @@ -206,11 +210,6 @@ class RegisterAction extends Action 'agree to the license.')); } else if ($email && !Validate::email($email, common_config('email', 'check_domain'))) { $this->showForm(_('Not a valid email address.')); - } else if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - $this->showForm(_('Nickname must have only lowercase letters '. - 'and numbers and no spaces.')); } else if ($this->nicknameExists($nickname)) { $this->showForm(_('Nickname already in use. Try another one.')); } else if (!User::allowed_nickname($nickname)) { diff --git a/lib/common.php b/lib/common.php index cd4fbfb15a..d891807185 100644 --- a/lib/common.php +++ b/lib/common.php @@ -117,6 +117,17 @@ require_once 'markdown.php'; // XXX: other formats here +/** + * Avoid the NICKNAME_FMT constant; use the Nickname class instead. + * + * Nickname::DISPLAY_FMT is more suitable for inserting into regexes; + * note that it includes the [] and repeating bits, so should be wrapped + * directly in a capture paren usually. + * + * For validation, use Nickname::validate() etc. + * + * @deprecated + */ define('NICKNAME_FMT', VALIDATE_NUM.VALIDATE_ALPHA_LOWER); require_once INSTALLDIR.'/lib/util.php'; diff --git a/lib/util.php b/lib/util.php index f5a4dc87b8..c170c8380f 100644 --- a/lib/util.php +++ b/lib/util.php @@ -520,18 +520,15 @@ function common_user_cache_hash($user=false) /** * get canonical version of nickname for comparison * - * Currently this just runs strtolower(); more is needed. - * - * @fixme normalize punctuation chars where applicable - * @fixme reject invalid input - * * @param string $nickname * @return string + * + * @throws NicknameException on invalid input + * @deprecated call Nickname::normalize() directly. */ function common_canonical_nickname($nickname) { - // XXX: UTF-8 canonicalization (like combining chars) - return strtolower($nickname); + return Nickname::normalize($nickname); } /** @@ -553,8 +550,6 @@ function common_canonical_email($email) /** * Partial notice markup rendering step: build links to !group references. * - * @fixme use abstracted group nickname regex - * * @param string $text partially rendered HTML * @param Notice $notice in whose context we're working * @return string partially rendered HTML @@ -564,7 +559,8 @@ function common_render_content($text, $notice) $r = common_render_text($text); $id = $notice->profile_id; $r = common_linkify_mentions($r, $notice); - $r = preg_replace('/(^|[\s\.\,\:\;]+)!([A-Za-z0-9]{1,64})/e', "'\\1!'.common_group_link($id, '\\2')", $r); + $r = preg_replace('/(^|[\s\.\,\:\;]+)!(' . Nickname::DISPLAY_FMT . ')/e', + "'\\1!'.common_group_link($id, '\\2')", $r); return $r; } @@ -625,11 +621,19 @@ function common_linkify_mention($mention) } /** - * @fixme use NICKNAME_FMT more consistently + * Find @-mentions in the given text, using the given notice object as context. + * References will be resolved with common_relative_profile() against the user + * who posted the notice. + * + * Note the return data format is internal, to be used for building links and + * such. Should not be used directly; rather, call common_linkify_mentions(). * * @param string $text * @param Notice $notice notice in whose context we're building links + * * @return array + * + * @access private */ function common_find_mentions($text, $notice) { @@ -665,12 +669,12 @@ function common_find_mentions($text, $notice) } } - preg_match_all('/^T ([A-Z0-9]{1,64}) /', + preg_match_all('/^T (' . Nickname::DISPLAY_FMT . ') /', $text, $tmatches, PREG_OFFSET_CAPTURE); - preg_match_all('/(?:^|\s+)@(['.NICKNAME_FMT.']{1,64})/', + preg_match_all('/(?:^|\s+)@(' . Nickname::DISPLAY_FMT . ')\b/', $text, $atmatches, PREG_OFFSET_CAPTURE); @@ -678,7 +682,12 @@ function common_find_mentions($text, $notice) $matches = array_merge($tmatches[1], $atmatches[1]); foreach ($matches as $match) { - $nickname = common_canonical_nickname($match[0]); + try { + $nickname = Nickname::normalize($match[0]); + } catch (NicknameException $e) { + // Bogus match? Drop it. + continue; + } // Try to get a profile for this nickname. // Start with conversation context, then go to @@ -1038,6 +1047,13 @@ function common_valid_profile_tag($str) return preg_match('/^[A-Za-z0-9_\-\.]{1,64}$/', $str); } +/** + * + * @param $sender_id + * @param $nickname + * @return + * @access private + */ function common_group_link($sender_id, $nickname) { $sender = Profile::staticGet($sender_id); diff --git a/plugins/Facebook/FBConnectAuth.php b/plugins/Facebook/FBConnectAuth.php index d6d3786261..84d51578f1 100644 --- a/plugins/Facebook/FBConnectAuth.php +++ b/plugins/Facebook/FBConnectAuth.php @@ -257,13 +257,10 @@ class FBConnectauthAction extends Action } } - $nickname = $this->trimmed('newname'); - - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.')); - return; + try { + $nickname = Nickname::normalize($this->trimmed('newname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); } if (!User::allowed_nickname($nickname)) { @@ -447,9 +444,7 @@ class FBConnectauthAction extends Action function isNewNickname($str) { - if (!Validate::string($str, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($str)) { return false; } if (!User::allowed_nickname($str)) { diff --git a/plugins/Mapstraction/MapstractionPlugin.php b/plugins/Mapstraction/MapstractionPlugin.php index d5261d8bc7..5ad25763e7 100644 --- a/plugins/Mapstraction/MapstractionPlugin.php +++ b/plugins/Mapstraction/MapstractionPlugin.php @@ -67,10 +67,10 @@ class MapstractionPlugin extends Plugin { $m->connect(':nickname/all/map', array('action' => 'allmap'), - array('nickname' => '['.NICKNAME_FMT.']{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect(':nickname/map', array('action' => 'usermap'), - array('nickname' => '['.NICKNAME_FMT.']{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); return true; } diff --git a/plugins/Mapstraction/map.php b/plugins/Mapstraction/map.php index 50ff82b67e..dbba4edd0c 100644 --- a/plugins/Mapstraction/map.php +++ b/plugins/Mapstraction/map.php @@ -53,7 +53,7 @@ class MapAction extends OwnerDesignAction parent::prepare($args); $nickname_arg = $this->arg('nickname'); - $nickname = common_canonical_nickname($nickname_arg); + $nickname = Nickname::normalize($nickname_arg); // Permanent redirect on non-canonical nickname diff --git a/plugins/OpenID/finishopenidlogin.php b/plugins/OpenID/finishopenidlogin.php index 01dd61edb1..86dd1c669b 100644 --- a/plugins/OpenID/finishopenidlogin.php +++ b/plugins/OpenID/finishopenidlogin.php @@ -272,13 +272,10 @@ class FinishopenidloginAction extends Action } } - $nickname = $this->trimmed('newname'); - - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - // TRANS: OpenID plugin message. The entered new user name did not conform to the requirements. - $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.')); + try { + $nickname = Nickname::validate($this->trimmed('newname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); return; } @@ -463,9 +460,7 @@ class FinishopenidloginAction extends Action function isNewNickname($str) { - if (!Validate::string($str, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($str)) { return false; } if (!User::allowed_nickname($str)) { diff --git a/plugins/TwitterBridge/twitterauthorization.php b/plugins/TwitterBridge/twitterauthorization.php index 931a037230..bbe41bd438 100644 --- a/plugins/TwitterBridge/twitterauthorization.php +++ b/plugins/TwitterBridge/twitterauthorization.php @@ -441,12 +441,10 @@ class TwitterauthorizationAction extends Action } } - $nickname = $this->trimmed('newname'); - - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.')); + try { + $nickname = Nickname::normalize($this->trimmed('newname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); return; } @@ -619,9 +617,7 @@ class TwitterauthorizationAction extends Action function isNewNickname($str) { - if (!Validate::string($str, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { + if (!Nickname::isValid($str)) { return false; } if (!User::allowed_nickname($str)) { diff --git a/tests/NicknameTest.php b/tests/NicknameTest.php index 95af94098d..f1d9808228 100644 --- a/tests/NicknameTest.php +++ b/tests/NicknameTest.php @@ -17,18 +17,37 @@ require_once INSTALLDIR . '/lib/common.php'; class NicknameTest extends PHPUnit_Framework_TestCase { /** - * @dataProvider provider + * Basic test using Nickname::normalize() * + * @dataProvider provider */ - public function testBasic($input, $expected) + public function testBasic($input, $expected, $expectedException=null) { - $matches = array(); - // First problem: this is all manual, wtf! - if (preg_match('/^([' . NICKNAME_FMT . ']{1,64})$/', $input, $matches)) { - $norm = common_canonical_nickname($matches[1]); - $this->assertEquals($expected, $norm, "normalized input nickname: $input -> $norm"); + $exception = null; + $normalized = false; + try { + $normalized = Nickname::normalize($normalized); + } catch (NicknameException $e) { + $exception = $e; + } + + if ($expected === false) { + if ($expectedException) { + $this->assert($exception && $exception instanceof $expectedException, + "invalid input '$input' expected to fail with $expectedException, " . + "got " . get_class($exception) . ': ' . $exception->getMessage()); + } else { + $this->assert($normalized == false, + "invalid input '$input' expected to fail"); + } } else { - $this->assertEquals($expected, false, "invalid input nickname: $input"); + $msg = "normalized input nickname '$input' expected to normalize to '$expected', got "; + if ($exception) { + $msg .= get_class($exception) . ': ' . $exception->getMessage(); + } else { + $msg .= "'$normalized'"; + } + $this->assertEquals($expected, $norm, $msg); } } @@ -36,17 +55,38 @@ class NicknameTest extends PHPUnit_Framework_TestCase { return array( array('evan', 'evan'), + + // Case and underscore variants array('Evan', 'evan'), array('EVAN', 'evan'), array('ev_an', 'evan'), - array('ev.an', 'evan'), - array('ev/an', false), - array('ev an', false), - array('ev-an', false), - array('évan', false), // so far... - array('Évan', false), // so far... + array('E__V_an', 'evan'), array('evan1', 'evan1'), array('evan_1', 'evan1'), + array('0x20', '0x20'), + array('1234', '1234'), // should this be allowed though? :) + array('12__34', '1234'), + + // Some (currently) invalid chars... + array('^#@&^#@', false, 'NicknameInvalidException'), // all invalid :D + array('ev.an', false, 'NicknameInvalidException'), + array('ev/an', false, 'NicknameInvalidException'), + array('ev an', false, 'NicknameInvalidException'), + array('ev-an', false, 'NicknameInvalidException'), + + // Non-ASCII letters; currently not allowed, in future + // we'll add them at least with conversion to ASCII. + // Not much use until we have storage of display names, + // though. + array('évan', false, 'NicknameInvalidException'), // so far... + array('Évan', false, 'NicknameInvalidException'), // so far... + + // Length checks + array('', false, 'NicknameEmptyException'), + array('___', false, 'NicknameEmptyException'), + array('eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee'), // 64 chars + array('eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee_', 'eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee'), // the _ will be trimmed off, remaining valid + array('eeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeeee', false, 'NicknameTooLongException'), // 65 chars -- too long ); } } From fffc10a23084f1bb4b47de926dc1034c8f9ee9b8 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 14:46:10 -0800 Subject: [PATCH 05/10] Nickname class to encapsulate validation & common regexes for nickname formats. This provides initial infrastructure for decoupling display names from internal canonical names, but continues to have us storing and using the canonical forms. It should be/become possible to provide mixed-case and underscore-containing names in links, @-mention, !-group, etc, but we don't store those alternate forms generally. --- lib/nickname.php | 176 +++++++++++++++++++++++++++++++++++++++++ tests/NicknameTest.php | 8 +- 2 files changed, 180 insertions(+), 4 deletions(-) create mode 100644 lib/nickname.php diff --git a/lib/nickname.php b/lib/nickname.php new file mode 100644 index 0000000000..48269f3b9f --- /dev/null +++ b/lib/nickname.php @@ -0,0 +1,176 @@ +. + */ + +class Nickname +{ + /** + * Regex fragment for pulling an arbitrarily-formated nickname. + * + * Not guaranteed to be valid after normalization; run the string through + * Nickname::normalize() to get the canonical form, or Nickname::validate() + * if you just need to check if it's properly formatted. + * + * This and CANONICAL_FMT replace the old NICKNAME_FMT, but be aware + * that these should not be enclosed in []s. + */ + const DISPLAY_FMT = '[0-9a-zA-Z_]+'; + + /** + * Regex fragment for checking a canonical nickname. + * + * Any non-matching string is not a valid canonical/normalized nickname. + * Matching strings are valid and canonical form, but may still be + * unavailable for registration due to blacklisting et. + * + * Only the canonical forms should be stored as keys in the database; + * there are multiple possible denormalized forms for each valid + * canonical-form name. + * + * This and DISPLAY_FMT replace the old NICKNAME_FMT, but be aware + * that these should not be enclosed in []s. + */ + const CANONICAL_FMT = '[0-9a-z]{1,64}'; + + /** + * Maximum number of characters in a canonical-form nickname. + */ + const MAX_LEN = 64; + + /** + * Nice simple check of whether the given string is a valid input nickname, + * which can be normalized into an internally canonical form. + * + * Note that valid nicknames may be in use or reserved. + * + * @param string $str + * @return boolean + */ + public static function validate($str) + { + try { + self::normalize($str); + return true; + } catch (NicknameException $e) { + return false; + } + } + + /** + * 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 + * @return string Normalized canonical form of $str + * + * @throws NicknameException (base class) + * @throws NicknameInvalidException + * @throws NicknameEmptyException + * @throws NicknameTooLongException + */ + public static function normalize($str) + { + $str = trim($str); + $str = str_replace('_', '', $str); + $str = mb_strtolower($str); + + $len = mb_strlen($str); + if ($len < 1) { + throw new NicknameEmptyException(); + } else if ($len > self::MAX_LEN) { + throw new NicknameTooLongException(); + } + if (!self::isCanonical($str)) { + throw new NicknameInvalidException(); + } + + return $str; + } + + /** + * Is the given string a valid canonical nickname form? + * + * @param string $str + * @return boolean + */ + public static function isCanonical($str) + { + return preg_match('/^(?:' . self::CANONICAL_FMT . ')$/', $str); + } +} + +class NicknameException extends ClientException +{ + function __construct($msg=null, $code=400) + { + if ($msg === null) { + $msg = $this->defaultMessage(); + } + parent::__construct($msg, $code); + } + + /** + * Default localized message for this type of exception. + * @return string + */ + protected function defaultMessage() + { + return null; + } +} + +class NicknameInvalidException extends NicknameException { + /** + * Default localized message for this type of exception. + * @return string + */ + protected function defaultMessage() + { + // TRANS: Validation error in form for registration, profile and group settings, etc. + return _('Nickname must have only lowercase letters and numbers and no spaces.'); + } +} + +class NicknameEmptyException extends NicknameException +{ + /** + * Default localized message for this type of exception. + * @return string + */ + protected function defaultMessage() + { + // TRANS: Validation error in form for registration, profile and group settings, etc. + return _('Nickname cannot be empty.'); + } +} + +class NicknameTooLongException extends NicknameInvalidException +{ + /** + * Default localized message for this type of exception. + * @return string + */ + protected function defaultMessage() + { + // TRANS: Validation error in form for registration, profile and group settings, etc. + return sprintf(_m('Nickname cannot be more than %d character long.', + 'Nickname cannot be more than %d characters long.', + Nickname::MAX_LEN), + Nickname::MAX_LEN); + } +} diff --git a/tests/NicknameTest.php b/tests/NicknameTest.php index f1d9808228..a59cada7ad 100644 --- a/tests/NicknameTest.php +++ b/tests/NicknameTest.php @@ -26,18 +26,18 @@ class NicknameTest extends PHPUnit_Framework_TestCase $exception = null; $normalized = false; try { - $normalized = Nickname::normalize($normalized); + $normalized = Nickname::normalize($input); } catch (NicknameException $e) { $exception = $e; } if ($expected === false) { if ($expectedException) { - $this->assert($exception && $exception instanceof $expectedException, + $this->assertTrue($exception && $exception instanceof $expectedException, "invalid input '$input' expected to fail with $expectedException, " . "got " . get_class($exception) . ': ' . $exception->getMessage()); } else { - $this->assert($normalized == false, + $this->assertTrue($normalized == false, "invalid input '$input' expected to fail"); } } else { @@ -47,7 +47,7 @@ class NicknameTest extends PHPUnit_Framework_TestCase } else { $msg .= "'$normalized'"; } - $this->assertEquals($expected, $norm, $msg); + $this->assertEquals($expected, $normalized, $msg); } } From 82799f675f2afa067ecb46e829d9b93b79b773b2 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 15:04:21 -0800 Subject: [PATCH 06/10] Add Nickname test cases for @-reply regexes in common_find_mentions --- lib/util.php | 47 ++++++++++++++++++++++++++++++++---------- tests/NicknameTest.php | 19 +++++++++++++++++ 2 files changed, 55 insertions(+), 11 deletions(-) diff --git a/lib/util.php b/lib/util.php index c170c8380f..317a7aa42e 100644 --- a/lib/util.php +++ b/lib/util.php @@ -564,6 +564,16 @@ function common_render_content($text, $notice) return $r; } +/** + * Finds @-mentions within the partially-rendered text section and + * turns them into live links. + * + * Should generally not be called except from common_render_content(). + * + * @param string $text partially-rendered HTML + * @param Notice $notice in-progress or complete Notice object for context + * @return string partially-rendered HTML + */ function common_linkify_mentions($text, $notice) { $mentions = common_find_mentions($text, $notice); @@ -669,17 +679,7 @@ function common_find_mentions($text, $notice) } } - preg_match_all('/^T (' . Nickname::DISPLAY_FMT . ') /', - $text, - $tmatches, - PREG_OFFSET_CAPTURE); - - preg_match_all('/(?:^|\s+)@(' . Nickname::DISPLAY_FMT . ')\b/', - $text, - $atmatches, - PREG_OFFSET_CAPTURE); - - $matches = array_merge($tmatches[1], $atmatches[1]); + $matches = common_find_mentions_raw($text); foreach ($matches as $match) { try { @@ -753,6 +753,31 @@ function common_find_mentions($text, $notice) return $mentions; } +/** + * Does the actual regex pulls to find @-mentions in text. + * Should generally not be called directly; for use in common_find_mentions. + * + * @param string $text + * @return array of PCRE match arrays + */ +function common_find_mentions_raw($text) +{ + $tmatches = array(); + preg_match_all('/^T (' . Nickname::DISPLAY_FMT . ') /', + $text, + $tmatches, + PREG_OFFSET_CAPTURE); + + $atmatches = array(); + preg_match_all('/(?:^|\s+)@(' . Nickname::DISPLAY_FMT . ')\b/', + $text, + $atmatches, + PREG_OFFSET_CAPTURE); + + $matches = array_merge($tmatches[1], $atmatches[1]); + return $matches; +} + function common_render_text($text) { $r = htmlspecialchars($text); diff --git a/tests/NicknameTest.php b/tests/NicknameTest.php index a59cada7ad..f49aeba602 100644 --- a/tests/NicknameTest.php +++ b/tests/NicknameTest.php @@ -51,6 +51,25 @@ class NicknameTest extends PHPUnit_Framework_TestCase } } + /** + * Test on the regex matching used in common_find_mentions + * (testing on the full notice rendering is difficult as it needs + * to be able to pull from global state) + * + * @dataProvider provider + */ + public function testAtReply($input, $expected, $expectedException=null) + { + if ($expected == false) { + // nothing to do + } else { + $text = "@{$input} awesome! :)"; + $matches = common_find_mentions_raw($text); + $this->assertEquals(1, count($matches)); + $this->assertEquals($expected, Nickname::normalize($matches[0][0])); + } + } + static public function provider() { return array( From 8d3577da349a5c6e497d8ff57a367e04eb5084a8 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 15:11:07 -0800 Subject: [PATCH 07/10] Replace a couple plugin usages of NICKNAME_FMT with Nickname::normalize() --- .../actions/facebookfinishlogin.php | 21 +++++-------------- 1 file changed, 5 insertions(+), 16 deletions(-) diff --git a/plugins/FacebookBridge/actions/facebookfinishlogin.php b/plugins/FacebookBridge/actions/facebookfinishlogin.php index 2174c5ad4a..349acd7e22 100644 --- a/plugins/FacebookBridge/actions/facebookfinishlogin.php +++ b/plugins/FacebookBridge/actions/facebookfinishlogin.php @@ -324,12 +324,10 @@ class FacebookfinishloginAction extends Action } } - $nickname = $this->trimmed('newname'); - - if (!Validate::string($nickname, array('min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT))) { - $this->showForm(_m('Nickname must have only lowercase letters and numbers and no spaces.')); + try { + $nickname = Nickname::normalize($this->trimmed('newname')); + } catch (NicknameException $e) { + $this->showForm($e->getMessage()); return; } @@ -639,16 +637,7 @@ class FacebookfinishloginAction extends Action */ function isNewNickname($str) { - if ( - !Validate::string( - $str, - array( - 'min_length' => 1, - 'max_length' => 64, - 'format' => NICKNAME_FMT - ) - ) - ) { + if (!Nickname::isValid($str)) { return false; } From e03d2584aaa2f16805e859b328b6536874bb3b9f Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 16:02:35 -0800 Subject: [PATCH 08/10] Use Nickname::DISPLAY_FMT instead of manual regex fragments in router setup for nickname parameters. --- lib/router.php | 82 +++++++++++++++++++++++++------------------------- 1 file changed, 41 insertions(+), 41 deletions(-) diff --git a/lib/router.php b/lib/router.php index 47357ca085..7d675bb547 100644 --- a/lib/router.php +++ b/lib/router.php @@ -223,10 +223,10 @@ class Router $m->connect('notice/new', array('action' => 'newnotice')); $m->connect('notice/new?replyto=:replyto', array('action' => 'newnotice'), - array('replyto' => '[A-Za-z0-9_-]+')); + array('replyto' => Nickname::DISPLAY_FMT)); $m->connect('notice/new?replyto=:replyto&inreplyto=:inreplyto', array('action' => 'newnotice'), - array('replyto' => '[A-Za-z0-9_-]+'), + array('replyto' => Nickname::DISPLAY_FMT), array('inreplyto' => '[0-9]+')); $m->connect('notice/:notice/file', @@ -250,7 +250,7 @@ class Router array('id' => '[0-9]+')); $m->connect('message/new', array('action' => 'newmessage')); - $m->connect('message/new?to=:to', array('action' => 'newmessage'), array('to' => '[A-Za-z0-9_-]+')); + $m->connect('message/new?to=:to', array('action' => 'newmessage'), array('to' => Nickname::DISPLAY_FMT)); $m->connect('message/:message', array('action' => 'showmessage'), array('message' => '[0-9]+')); @@ -281,7 +281,7 @@ class Router foreach (array('edit', 'join', 'leave', 'delete') as $v) { $m->connect('group/:nickname/'.$v, array('action' => $v.'group'), - array('nickname' => '[a-zA-Z0-9]+')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect('group/:id/id/'.$v, array('action' => $v.'group'), array('id' => '[0-9]+')); @@ -290,20 +290,20 @@ class Router foreach (array('members', 'logo', 'rss', 'designsettings') as $n) { $m->connect('group/:nickname/'.$n, array('action' => 'group'.$n), - array('nickname' => '[a-zA-Z0-9]+')); + array('nickname' => Nickname::DISPLAY_FMT)); } $m->connect('group/:nickname/foaf', array('action' => 'foafgroup'), - array('nickname' => '[a-zA-Z0-9]+')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect('group/:nickname/blocked', array('action' => 'blockedfromgroup'), - array('nickname' => '[a-zA-Z0-9]+')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect('group/:nickname/makeadmin', array('action' => 'makeadmin'), - array('nickname' => '[a-zA-Z0-9]+')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect('group/:id/id', array('action' => 'groupbyid'), @@ -311,7 +311,7 @@ class Router $m->connect('group/:nickname', array('action' => 'showgroup'), - array('nickname' => '[a-zA-Z0-9]+')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect('group/', array('action' => 'groups')); $m->connect('group', array('action' => 'groups')); @@ -332,7 +332,7 @@ class Router $m->connect('api/statuses/friends_timeline/:id.:format', array('action' => 'ApiTimelineFriends', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json|rss|atom)')); $m->connect('api/statuses/home_timeline.:format', @@ -341,7 +341,7 @@ class Router $m->connect('api/statuses/home_timeline/:id.:format', array('action' => 'ApiTimelineHome', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json|rss|atom)')); $m->connect('api/statuses/user_timeline.:format', @@ -350,7 +350,7 @@ class Router $m->connect('api/statuses/user_timeline/:id.:format', array('action' => 'ApiTimelineUser', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json|rss|atom)')); $m->connect('api/statuses/mentions.:format', @@ -359,7 +359,7 @@ class Router $m->connect('api/statuses/mentions/:id.:format', array('action' => 'ApiTimelineMentions', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json|rss|atom)')); $m->connect('api/statuses/replies.:format', @@ -368,7 +368,7 @@ class Router $m->connect('api/statuses/replies/:id.:format', array('action' => 'ApiTimelineMentions', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json|rss|atom)')); $m->connect('api/statuses/retweeted_by_me.:format', @@ -389,7 +389,7 @@ class Router $m->connect('api/statuses/friends/:id.:format', array('action' => 'ApiUserFriends', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/statuses/followers.:format', @@ -398,7 +398,7 @@ class Router $m->connect('api/statuses/followers/:id.:format', array('action' => 'ApiUserFollowers', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/statuses/show.:format', @@ -441,7 +441,7 @@ class Router $m->connect('api/users/show/:id.:format', array('action' => 'ApiUserShow', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); // direct messages @@ -479,12 +479,12 @@ class Router $m->connect('api/friendships/create/:id.:format', array('action' => 'ApiFriendshipsCreate', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/friendships/destroy/:id.:format', array('action' => 'ApiFriendshipsDestroy', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); // Social graph @@ -541,17 +541,17 @@ class Router $m->connect('api/favorites/:id.:format', array('action' => 'ApiTimelineFavorites', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json|rss|atom)')); $m->connect('api/favorites/create/:id.:format', array('action' => 'ApiFavoriteCreate', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/favorites/destroy/:id.:format', array('action' => 'ApiFavoriteDestroy', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); // blocks @@ -561,7 +561,7 @@ class Router $m->connect('api/blocks/create/:id.:format', array('action' => 'ApiBlockCreate', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/blocks/destroy.:format', @@ -570,7 +570,7 @@ class Router $m->connect('api/blocks/destroy/:id.:format', array('action' => 'ApiBlockDestroy', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); // help @@ -606,7 +606,7 @@ class Router $m->connect('api/statusnet/groups/timeline/:id.:format', array('action' => 'ApiTimelineGroup', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json|rss|atom)')); $m->connect('api/statusnet/groups/show.:format', @@ -615,12 +615,12 @@ class Router $m->connect('api/statusnet/groups/show/:id.:format', array('action' => 'ApiGroupShow', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/statusnet/groups/join.:format', array('action' => 'ApiGroupJoin', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/statusnet/groups/join/:id.:format', @@ -629,7 +629,7 @@ class Router $m->connect('api/statusnet/groups/leave.:format', array('action' => 'ApiGroupLeave', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/statusnet/groups/leave/:id.:format', @@ -646,7 +646,7 @@ class Router $m->connect('api/statusnet/groups/list/:id.:format', array('action' => 'ApiGroupList', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json|rss|atom)')); $m->connect('api/statusnet/groups/list_all.:format', @@ -659,7 +659,7 @@ class Router $m->connect('api/statusnet/groups/membership/:id.:format', array('action' => 'ApiGroupMembership', - 'id' => '[a-zA-Z0-9]+', + 'id' => Nickname::DISPLAY_FMT, 'format' => '(xml|json)')); $m->connect('api/statusnet/groups/create.:format', @@ -692,7 +692,7 @@ class Router $m->connect('api/statusnet/app/service/:id.xml', array('action' => 'ApiAtomService', - 'id' => '[a-zA-Z0-9]+')); + 'id' => Nickname::DISPLAY_FMT)); $m->connect('api/statusnet/app/service.xml', array('action' => 'ApiAtomService')); @@ -789,54 +789,54 @@ class Router 'replies', 'inbox', 'outbox', 'microsummary', 'hcard') as $a) { $m->connect(':nickname/'.$a, array('action' => $a), - array('nickname' => '[a-zA-Z0-9]{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); } foreach (array('subscriptions', 'subscribers') as $a) { $m->connect(':nickname/'.$a.'/:tag', array('action' => $a), array('tag' => '[a-zA-Z0-9]+', - 'nickname' => '[a-zA-Z0-9]{1,64}')); + 'nickname' => Nickname::DISPLAY_FMT)); } foreach (array('rss', 'groups') as $a) { $m->connect(':nickname/'.$a, array('action' => 'user'.$a), - array('nickname' => '[a-zA-Z0-9]{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); } foreach (array('all', 'replies', 'favorites') as $a) { $m->connect(':nickname/'.$a.'/rss', array('action' => $a.'rss'), - array('nickname' => '[a-zA-Z0-9]{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); } $m->connect(':nickname/favorites', array('action' => 'showfavorites'), - array('nickname' => '[a-zA-Z0-9]{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect(':nickname/avatar/:size', array('action' => 'avatarbynickname'), array('size' => '(original|96|48|24)', - 'nickname' => '[a-zA-Z0-9]{1,64}')); + 'nickname' => Nickname::DISPLAY_FMT)); $m->connect(':nickname/tag/:tag/rss', array('action' => 'userrss'), - array('nickname' => '[a-zA-Z0-9]{1,64}'), + array('nickname' => Nickname::DISPLAY_FMT), array('tag' => '[\pL\pN_\-\.]{1,64}')); $m->connect(':nickname/tag/:tag', array('action' => 'showstream'), - array('nickname' => '[a-zA-Z0-9]{1,64}'), + array('nickname' => Nickname::DISPLAY_FMT), array('tag' => '[\pL\pN_\-\.]{1,64}')); $m->connect(':nickname/rsd.xml', array('action' => 'rsd'), - array('nickname' => '[a-zA-Z0-9]{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); $m->connect(':nickname', array('action' => 'showstream'), - array('nickname' => '[a-zA-Z0-9]{1,64}')); + array('nickname' => Nickname::DISPLAY_FMT)); } // user stuff From 3f0557aa8efa715e288c731178a27e8d4914a1a7 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 16:44:01 -0800 Subject: [PATCH 09/10] General code safety: validate input and escape SQL strings in common_relative_profile() --- lib/util.php | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/util.php b/lib/util.php index 317a7aa42e..42762b22fb 100644 --- a/lib/util.php +++ b/lib/util.php @@ -1118,17 +1118,20 @@ function common_group_link($sender_id, $nickname) * * @param $sender the user or profile in whose context we're looking * @param string $nickname validated nickname of - * @param $dt unused mystery parameter. + * @param $dt unused mystery parameter; in Notice reply-to handling a timestamp is passed. * * @return Profile or null */ function common_relative_profile($sender, $nickname, $dt=null) { + // Will throw exception on invalid input. + $nickname = Nickname::normalize($nickname); + // Try to find profiles this profile is subscribed to that have this nickname $recipient = new Profile(); // XXX: use a join instead of a subquery - $recipient->whereAdd('EXISTS (SELECT subscribed from subscription where subscriber = '.$sender->id.' and subscribed = id)', 'AND'); - $recipient->whereAdd("nickname = '" . trim($nickname) . "'", 'AND'); + $recipient->whereAdd('EXISTS (SELECT subscribed from subscription where subscriber = '.intval($sender->id).' and subscribed = id)', 'AND'); + $recipient->whereAdd("nickname = '" . $recipient->escape($nickname) . "'", 'AND'); if ($recipient->find(true)) { // XXX: should probably differentiate between profiles with // the same name by date of most recent update @@ -1137,8 +1140,8 @@ function common_relative_profile($sender, $nickname, $dt=null) // Try to find profiles that listen to this profile and that have this nickname $recipient = new Profile(); // XXX: use a join instead of a subquery - $recipient->whereAdd('EXISTS (SELECT subscriber from subscription where subscribed = '.$sender->id.' and subscriber = id)', 'AND'); - $recipient->whereAdd("nickname = '" . trim($nickname) . "'", 'AND'); + $recipient->whereAdd('EXISTS (SELECT subscriber from subscription where subscribed = '.intval($sender->id).' and subscriber = id)', 'AND'); + $recipient->whereAdd("nickname = '" . $recipient->escape($nickname) . "'", 'AND'); if ($recipient->find(true)) { // XXX: should probably differentiate between profiles with // the same name by date of most recent update From 3be352551a979cc3e89e0a2b8f950949b932a304 Mon Sep 17 00:00:00 2001 From: Brion Vibber Date: Mon, 29 Nov 2010 16:44:40 -0800 Subject: [PATCH 10/10] Normalize username strings in command parsing --- lib/command.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/command.php b/lib/command.php index ae69f04a13..2a8075e7ba 100644 --- a/lib/command.php +++ b/lib/command.php @@ -139,7 +139,7 @@ class Command { $user = null; if (Event::handle('StartCommandGetUser', array($this, $arg, &$user))) { - $user = User::staticGet('nickname', $arg); + $user = User::staticGet('nickname', Nickname::normalize($arg)); } Event::handle('EndCommandGetUser', array($this, $arg, &$user)); if (!$user){